Skip to content

Conversation

@pbkompasz
Copy link
Contributor

@pbkompasz pbkompasz commented Jun 27, 2025

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

About #231

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • PR title matches follow: (Feature|Bug|Chore) Task Name
  • My code follows the style guidelines of this project
  • I have followed all the instructions described in the initial task (check Definitions of Done)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added reference to a related issue in the repository
  • I have added a detailed description of the changes proposed in the pull request. I am as descriptive as possible, assisting reviewers as much as possible.
  • I have added screenshots related to my pull request (for frontend tasks)
  • I have pasted a gif showing the feature.
  • @mentions of the person or team responsible for reviewing proposed changes

Description by Korbit AI

What change is being made?

Introduce a feature for creating Community Pools including new components, hooks, assets, and routes to facilitate the pool creation process.

Why are these changes being made?

These changes implement the functionality to allow users to create a "Good Collective" within the application, breaking down the pool creation into a step-by-step workflow. Each step is managed via dedicated components for increased organizational clarity and reusability. This approach enhances the application by enabling new user interactions and aligning with the application's goal to facilitate community-driven projects efficiently.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Functionality Non-functional Image Upload ▹ view ✅ Fix detected
Functionality Missing Wallet Authentication Check ▹ view
Functionality Typo in Step Description ▹ view
Design Rigid Step Management ▹ view ✅ Fix detected
Functionality Missing Form Validation Logic ▹ view ✅ Fix detected
Design Redundant Styling Properties ▹ view ✅ Fix detected
Functionality Missing key prop in mapped components ▹ view ✅ Fix detected
Files scanned
File Path Reviewed
packages/app/src/components/create/CreateContract/4ReviewLaunch.tsx
packages/app/src/components/create/CreateContract/2ProjectDetails.tsx
packages/app/src/components/create/CreateContract/3PoolConfiguration.tsx
packages/app/src/pages/CreateGoodCollectivePage.tsx
packages/app/src/components/create/CreateContract/CreateContract.tsx
packages/app/src/components/create/CreateGoodCollective.tsx
packages/app/src/assets/index.ts
packages/app/src/App.tsx
packages/app/src/components/create/SelectCollectiveType.tsx
packages/app/src/components/create/Welcome.tsx
packages/app/src/components/create/CreateContract/1GetStarted.tsx
packages/app/src/pages/HomePage.tsx

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

<HStack style={{ width: '100%', justifyContent: 'space-between' }}>
<p style={{ color: step === 2 ? 'white' : 'grey' }}>Get Started(1/4)</p>
<p style={{ color: step === 3 ? 'white' : 'grey' }}>Create Pool(2/4)</p>
<p style={{ color: step === 4 ? 'white' : 'grey' }}>Pool Configuraiton(3/4)</p>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in Step Description category Functionality

Tell me more
What is the issue?

There is a typo in the word 'Configuraiton'. It should be spelled as 'Configuration'.

Why this matters

If the typo in 'Pool Configuraiton' is not corrected to 'Pool Configuration', it may confuse users or give an impression of a lack of attention to detail in the application interface.

Suggested change ∙ Feature Preview

Replace the misspelled word with the correct spelling:

<p style={{ color: step === 4 ? 'white' : 'grey' }}>Pool Configuration(3/4)</p>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines 44 to 50
{poolTypes.map((poolType) => (
<Pressable
disabled={poolType.disabled}
onPress={() => {
setType(poolType.id);
onStepForward();
}}>

This comment was marked as resolved.

Comment on lines 7 to 8
<CreateGoodCollective />
{/* TODO Show has to log in wallet first */}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Wallet Authentication Check category Functionality

Tell me more
What is the issue?

The page is not implementing the wallet authentication check mentioned in the TODO comment, which is critical for creating community pools.

Why this matters

Without wallet authentication, users could reach the community pool creation interface but fail to complete transactions, leading to a poor user experience and potential errors.

Suggested change ∙ Feature Preview

Implement wallet authentication check and conditional rendering:

import { useWallet } from '../hooks/useWallet';

function CreateGoodCollectivePage() {
  const { isConnected } = useWallet();

  return (
    <Layout>
      {isConnected ? (
        <CreateGoodCollective />
      ) : (
        <div>Please connect your wallet to create a community pool</div>
      )}
    </Layout>
  );
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines 18 to 21
{step === 2 && <GetStarted />}
{step === 3 && <ProjectDetails />}
{step === 4 && <PoolConfiguration />}
{step === 5 && <ReviewLaunch />}

This comment was marked as resolved.

Comment on lines 18 to 21
<Input />
<FormControl.ErrorMessage leftIcon={<WarningOutlineIcon size="xs" />}>
Something is wrong.
</FormControl.ErrorMessage>

This comment was marked as resolved.

Comment on lines 50 to 55
<Box backgroundColor="white">
<Center>
icon
<Text>Click to upload</Text>
</Center>
</Box>

This comment was marked as resolved.

Comment on lines 30 to 40
<Radio.Group
name="donationFrequency"
value={value}
onChange={(v) => {
setValue(v);
console.log(v);
}}
style={{ flexDirection: 'column' }}
flexDir="column"
justifyContent="space-between"
flexBasis={{ lg: '100%', md: '70%', sm: '100%', base: '100%' }}>

This comment was marked as resolved.

@sirpy
Copy link
Contributor

sirpy commented Jun 30, 2025

@pbkompasz
I enabled preview deployment.
please check the AI code review there are some useful comments there

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Jul 25, 2025

@pbkompasz Alright, please make this priority to finalize before doing the other issue :)

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Aug 4, 2025

@pbkompasz Hi, whats the status of this? is it ready for review (because I see its still in draft..)

@pbkompasz
Copy link
Contributor Author

@L03TJ3 It's ready

@pbkompasz pbkompasz marked this pull request as ready for review August 5, 2025 11:11
@pbkompasz pbkompasz changed the title [WIP] feat: Create Community Pools feat: Create Community Pools Aug 5, 2025
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pbkompasz - I've reviewed your changes - here's some feedback:

  • There's a lot of repeated form validation and state management across GetStarted, ProjectDetails, and PoolConfiguration—consider extracting shared validation logic or a custom hook to reduce duplication.
  • Numeric step values are hardcoded throughout CreatePoolProvider and components; centralize them as named constants or an enum to avoid magic numbers and improve readability.
  • The CreateContract component uses ad-hoc arrays and conditional rendering for each step—simplify this by maintaining a single step-to-component configuration mapping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There's a lot of repeated form validation and state management across GetStarted, ProjectDetails, and PoolConfiguration—consider extracting shared validation logic or a custom hook to reduce duplication.
- Numeric step values are hardcoded throughout CreatePoolProvider and components; centralize them as named constants or an enum to avoid magic numbers and improve readability.
- The CreateContract component uses ad-hoc arrays and conditional rendering for each step—simplify this by maintaining a single step-to-component configuration mapping.

## Individual Comments

### Comment 1
<location> `packages/app/src/pages/HomePage.tsx:90` </location>
<code_context>
     }
   };

+  const redirectToCreateCollective = () => {
+    if (!isConnected) {
+      setShowWarningMessage(true);
+    }
+    setShowWarningMessage(false);
+    navigate(`/create`);
+  };
</code_context>

<issue_to_address>
Warning message is immediately hidden after being shown.

The warning is set to true but then immediately reset to false, so it won't display. Please adjust the logic to ensure the warning remains visible when needed, such as by returning early after setting it.
</issue_to_address>

### Comment 2
<location> `packages/app/src/components/create/CreateContract/1GetStarted.tsx:88` </location>
<code_context>
+
+    // Pool Name* - 100 character max
+    // (Can have spaces, no special characters allowed, 0-9/a-z/A-Z)
+    if (!projectName) {
+      if (checkEmpty) {
+        currErrors.projectName = 'Project name is required';
+        pass = false;
+      }
+    } else if (projectName.length > 30) {
+      currErrors.projectName = 'Project name length (max 30 characters)';
+      pass = false;
+    } else if (!/^[a-zA-Z0-9]*$/.test(projectName)) {
+      currErrors.projectName = 'Project name cannot contain special characters';
+      pass = false;
</code_context>

<issue_to_address>
Project name validation does not allow spaces.

The regex currently disallows spaces, which contradicts the comment. Update the regex to permit spaces if they are intended to be allowed.
</issue_to_address>

### Comment 3
<location> `packages/app/src/components/create/CreateContract/1GetStarted.tsx:128` </location>
<code_context>
+    const validateImg = async (
</code_context>

<issue_to_address>
Image validation may not work as intended for remote images.

The current approach may fail for remote images due to CORS or loading issues. Please consider a more reliable validation method or add error handling for failed image loads.
</issue_to_address>

### Comment 4
<location> `packages/app/src/components/create/CreateContract/3PoolConfiguration.tsx:99` </location>
<code_context>
+    };
+    let pass = true;
+
+    const regex = /^[a-zA-Z0-9]+$/;
+    const addresses = poolRecipients.split(',');
+    for (let i = 0; i < addresses.length; i++) {
+      if (!regex.test(addresses[i])) {
+        currErrors.poolRecipients = 'Invalid format';
+        break;
</code_context>

<issue_to_address>
Pool recipient address validation is too restrictive.

The current regex does not allow for the '0x' prefix or restrict to valid hexadecimal characters. Please update the regex to accurately validate Ethereum addresses.
</issue_to_address>

### Comment 5
<location> `packages/app/src/components/create/CreateContract/3PoolConfiguration.tsx:108` </location>
<code_context>
+      }
+    }
+
+    if (form.expectedMembers && expectedMembers !== addresses.length) {
+      currErrors.poolRecipients = 'Number of addresses must match the amount of members';
+    }
</code_context>

<issue_to_address>
Comparison between expectedMembers and addresses.length may be unreliable if addresses are empty strings.

Filter out empty strings from addresses before comparing lengths to ensure accurate validation.

Suggested implementation:

```typescript
    const addresses = poolRecipients.split(',');
    const filteredAddresses = addresses.filter(addr => addr.trim() !== '');
    for (let i = 0; i < addresses.length; i++) {
      if (!regex.test(addresses[i])) {
        currErrors.poolRecipients = 'Invalid format';
        break;
      }
    }


      );
      if (typeof resp === 'string') setEnsName(resp);
    })();
  }, [ensName, managerAddress]);

  const validate = () => {
    const currErrors: FormError = {
      customClaimFrequency: '',
      poolRecipients: '',
      claimAmountPerWeek: '',

```

```typescript
    if (form.expectedMembers && expectedMembers !== filteredAddresses.length) {
      currErrors.poolRecipients = 'Number of addresses must match the amount of members';
    }

```
</issue_to_address>

### Comment 6
<location> `packages/app/src/components/create/CreateContract/3PoolConfiguration.tsx:133` </location>
<code_context>
+  };
+
+  const submitForm = () => {
+    if (validate())
+      submitPartial({
+        poolManagerFeeType,
+        claimFrequency: claimFrequency !== 2 ? claimFrequency : customClaimFrequency,
+        joinStatus,
+        maximumMembers,
+        managerAddress,
+        poolRecipients,
+        managerFeePercentage,
+        claimAmountPerWeek: claimAmountPerWeek,
+        expectedMembers,
+      });
+    nextStep();
+  };
+
</code_context>

<issue_to_address>
nextStep() is called regardless of validation result.

Since nextStep() is outside the validation check, users can proceed even if validation fails. Consider moving nextStep() inside the if block to prevent this.
</issue_to_address>

### Comment 7
<location> `packages/app/src/components/create/CreateContract/4ReviewLaunch.tsx:94` </location>
<code_context>
+            Socials
+          </Text>
+          <HStack space={2}>
+            {socials.map((social) => (
+              <Box backgroundColor="gray.100" width={10} height={10} justifyContent="center" alignItems="center">
+                <img width={24} src={social.icon} />
+              </Box>
+            ))}
</code_context>

<issue_to_address>
Missing key prop in socials.map rendering.

Add a unique key prop to each element in the socials.map to prevent React warnings and rendering issues.
</issue_to_address>

### Comment 8
<location> `packages/app/src/components/create/SelectCollectiveType.tsx:92` </location>
<code_context>
+                </Badge>
+              )}
+            </VStack>
+            <Box w="1/6">{!poolType.disabled && <Checkbox value={'true'} />}</Box>
+          </HStack>
+        </Pressable>
</code_context>

<issue_to_address>
Checkbox is rendered but not controlled.

Make the Checkbox controlled to reflect the selected state, or remove it if it's unnecessary.

Suggested implementation:

```typescript
            <Box w="1/6">
              {!poolType.disabled && (
                <Checkbox
                  value={'true'}
                  isChecked={selectedPoolType === poolType.id}
                  onChange={() => onSelectPoolType(poolType.id)}
                />
              )}
            </Box>

```

To fully implement this, you need to:
1. Ensure that `selectedPoolType` (the currently selected pool type's id) and `onSelectPoolType` (a function to update the selected pool type) are passed as props to this component, or are defined in this component's state if not already present.
2. If this is a single-select scenario, `selectedPoolType` should be a string or number representing the selected id. If multi-select, adjust accordingly.
</issue_to_address>

### Comment 9
<location> `packages/app/src/components/create/Welcome.tsx:100` </location>
<code_context>
+              alignItems: 'flex-start',
+            }}
+            leftIcon={<WarningOutlineIcon size="xs" mt={1} />}>
+            You must aknowledge...
+          </FormControl.ErrorMessage>
+        </VStack>
</code_context>

<issue_to_address>
Typo in error message: 'aknowledge' should be 'acknowledge'.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
            You must aknowledge...
=======
            You must acknowledge...
>>>>>>> REPLACE

</suggested_fix>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +90 to +94
const redirectToCreateCollective = () => {
if (!isConnected) {
setShowWarningMessage(true);
}
setShowWarningMessage(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Warning message is immediately hidden after being shown.

The warning is set to true but then immediately reset to false, so it won't display. Please adjust the logic to ensure the warning remains visible when needed, such as by returning early after setting it.

Comment on lines +88 to +96
if (!projectName) {
if (checkEmpty) {
currErrors.projectName = 'Project name is required';
pass = false;
}
} else if (projectName.length > 30) {
currErrors.projectName = 'Project name length (max 30 characters)';
pass = false;
} else if (!/^[a-zA-Z0-9]*$/.test(projectName)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Project name validation does not allow spaces.

The regex currently disallows spaces, which contradicts the comment. Update the regex to permit spaces if they are intended to be allowed.

Comment on lines +128 to +137
const validateImg = async (
imgUrl: string,
maxSize: number,
maxWidth: number,
maxHeight: number,
imgType: 'logo' | 'coverPhoto'
) => {
const response = await fetch(imgUrl, { method: 'HEAD' });
const contentLength = response.headers.get('content-length');
const size = contentLength ? parseInt(contentLength, 10) : null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Image validation may not work as intended for remote images.

The current approach may fail for remote images due to CORS or loading issues. Please consider a more reliable validation method or add error handling for failed image loads.

Comment on lines +99 to +102
const regex = /^[a-zA-Z0-9]+$/;
const addresses = poolRecipients.split(',');
for (let i = 0; i < addresses.length; i++) {
if (!regex.test(addresses[i])) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Pool recipient address validation is too restrictive.

The current regex does not allow for the '0x' prefix or restrict to valid hexadecimal characters. Please update the regex to accurately validate Ethereum addresses.

}
}

if (form.expectedMembers && expectedMembers !== addresses.length) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Comparison between expectedMembers and addresses.length may be unreliable if addresses are empty strings.

Filter out empty strings from addresses before comparing lengths to ensure accurate validation.

Suggested implementation:

    const addresses = poolRecipients.split(',');
    const filteredAddresses = addresses.filter(addr => addr.trim() !== '');
    for (let i = 0; i < addresses.length; i++) {
      if (!regex.test(addresses[i])) {
        currErrors.poolRecipients = 'Invalid format';
        break;
      }
    }


      );
      if (typeof resp === 'string') setEnsName(resp);
    })();
  }, [ensName, managerAddress]);

  const validate = () => {
    const currErrors: FormError = {
      customClaimFrequency: '',
      poolRecipients: '',
      claimAmountPerWeek: '',
    if (form.expectedMembers && expectedMembers !== filteredAddresses.length) {
      currErrors.poolRecipients = 'Number of addresses must match the amount of members';
    }

alignItems: 'flex-start',
}}
leftIcon={<WarningOutlineIcon size="xs" mt={1} />}>
You must aknowledge...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (typo): Typo in error message: 'aknowledge' should be 'acknowledge'.

Suggested change
You must aknowledge...
You must acknowledge...

import { InfoIconOrange } from '../assets';

const WarningBox = ({ content, explanationProps = {} }: any) => {
const Explanation = content.Explanation;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change
const Explanation = content.Explanation;
const {Explanation} = content;


ExplanationObject destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

Comment on lines +112 to +117
if (!logo) {
if (checkEmpty) {
currErrors.logo = 'Logo is required';
pass = false;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)

Suggested change
if (!logo) {
if (checkEmpty) {
currErrors.logo = 'Logo is required';
pass = false;
}
}
if (!logo && checkEmpty) {
currErrors.logo = 'Logo is required';
pass = false;
}


ExplanationReading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.

style={isDesktopView ? styles.desktopStep : styles.mobileStep}
fontWeight="600"
color={step === 5 ? 'white' : 'gray.600'}>
Review & Launch(4/4)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Simplify binary operation (binary-operator-identity)

Suggested change
Review & Launch(4/4)
Review & Launch(1)


ExplanationSome identities can be applied to simplify certain binary operations. For example
writing x - x is a complicated way of saying 0.

Comment on lines +30 to +31
const { address: maybeAddress } = useAccount();
const { chain } = useAccount();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Combine destructure assignments. (combine-object-destructuring)

Suggested change
const { address: maybeAddress } = useAccount();
const { chain } = useAccount();
const {address: maybeAddress, chain} = useAccount();


ExplanationObject destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Functionality Review Mode Field Editability ▹ view
Design Mixed Styling Approaches ▹ view
Functionality Missing Donation Handler ▹ view
Functionality Incomplete Share Project Feature ▹ view
Performance Inefficient Step Rendering Logic ▹ view
Functionality Incorrect Wallet Connection Flow ▹ view
Performance Unprotected Async Operation in Launch Button ▹ view
Functionality Incorrect Value Reuse ▹ view
Security Unsanitized Wallet Address Display ▹ view
Design Inconsistent Component Library Usage ▹ view
Files scanned
File Path Reviewed
packages/app/src/hooks/useCreatePool/index.ts
packages/app/src/hooks/useCreatePool/util.ts
packages/app/src/pages/CreateGoodCollectivePage.tsx
packages/app/src/lib/defaults.ts
packages/app/src/components/create/CreateGoodCollective.tsx
packages/app/src/components/ActionButton.tsx
packages/app/src/hooks/useCreatePool/useCreatePool.ts
packages/app/src/components/FileUpload.tsx
packages/app/src/components/WarningBox.tsx
packages/app/src/components/create/CreateContract/CreateContract.tsx
packages/app/src/components/create/Success.tsx
packages/app/src/assets/index.ts
packages/app/src/App.tsx
packages/app/src/components/create/SelectCollectiveType.tsx
packages/app/src/components/create/Welcome.tsx
packages/app/src/pages/HomePage.tsx
packages/app/src/hooks/useCreatePool/CreatePoolContext.tsx
packages/app/src/components/create/CreateContract/4ReviewLaunch.tsx
packages/app/src/components/create/CreateContract/2ProjectDetails.tsx
packages/app/src/components/create/CreateContract/1GetStarted.tsx
packages/app/src/components/create/CreateContract/3PoolConfiguration.tsx

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

Comment on lines +10 to +14
<VStack
space={4}
style={{ minWidth: isDesktopView ? '600px' : '150px' }}
width={isDesktopView ? '1/2' : 'full'}
marginX="auto">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixed Styling Approaches category Design

Tell me more
What is the issue?

Mixing inline styles with native-base props creates inconsistent styling patterns.

Why this matters

Using multiple styling approaches makes it harder to maintain consistent styling and complicates theme changes across the application.

Suggested change ∙ Feature Preview

Use native-base props consistently:

<VStack
  space={4}
  minW={isDesktopView ? "600px" : "150px"}
  w={isDesktopView ? "1/2" : "full"}
  mx="auto">
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +47 to +48
{/* TODO */}
<HStack>...</HStack>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete Share Project Feature category Functionality

Tell me more
What is the issue?

Missing implementation for sharing project link functionality despite UI elements indicating this feature should be present.

Why this matters

Users will not be able to share their project, which is a critical feature after successful project deployment. This defeats the purpose of the 'Share Project Link' section.

Suggested change ∙ Feature Preview

Implement the project sharing functionality. A basic implementation could look like:

<HStack space={4} alignItems="center">
  <Input value={projectLink} isReadOnly />
  <IconButton
    icon={<ShareIcon />}
    onPress={() => handleShareProject(projectLink)}
    aria-label="Share project"
  />
</HStack>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Now it's time to fund your GoodCollective!
</Text>
<Box width="full">
<ActionButton text="Donate" bg="goodPurple.400" textColor="white" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Donation Handler category Functionality

Tell me more
What is the issue?

Donate button lacks onClick handler for donation functionality.

Why this matters

Users clicking the donate button will experience no action, preventing them from funding the GoodCollective as prompted by the UI.

Suggested change ∙ Feature Preview

Add an onClick handler to the ActionButton:

<ActionButton 
  text="Donate" 
  bg="goodPurple.400" 
  textColor="white" 
  onClick={handleDonation}
/>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +12 to +15
{step === 0 && <Welcome />}
{step === 1 && <SelectType />}
{step >= 2 && step <= 5 && <CreateContract />}
{step === 6 && <Success />}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inefficient Step Rendering Logic category Performance

Tell me more
What is the issue?

Multiple conditional renders causing unnecessary component re-evaluations on every step change.

Why this matters

Each step change triggers evaluation of all conditions, leading to unnecessary JavaScript execution overhead as the application scales.

Suggested change ∙ Feature Preview

Use a switch statement or an object mapping to render components more efficiently. Example:

const stepComponents = {
  0: Welcome,
  1: SelectType,
  6: Success
};

const getComponent = () => {
  if (step >= 2 && step <= 5) return CreateContract;
  return stepComponents[step] || null;
}

return (
  <div>
    {React.createElement(getComponent())}
  </div>
);
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +90 to +96
const redirectToCreateCollective = () => {
if (!isConnected) {
setShowWarningMessage(true);
}
setShowWarningMessage(false);
navigate(`/create`);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect Wallet Connection Flow category Functionality

Tell me more
What is the issue?

The function always navigates to '/create' and sets showWarningMessage to false, regardless of wallet connection status. This defeats the purpose of the connection check.

Why this matters

Users without a connected wallet will briefly see the warning message before being incorrectly redirected to the create page, leading to potential errors or undefined behavior in the creation flow.

Suggested change ∙ Feature Preview
const redirectToCreateCollective = () => {
    if (!isConnected) {
      setShowWarningMessage(true);
      return;
    }
    setShowWarningMessage(false);
    navigate(`/create`);
  };
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +218 to +227
onPress={async () => {
setShowModal(true);
const resp = await createPool();
if (!resp) {
setShowErrorModal(true);
} else {
setShowModal(false);
nextStep();
}
}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unprotected Async Operation in Launch Button category Performance

Tell me more
What is the issue?

The async operation handling in the Launch Pool button doesn't properly manage loading states or prevent multiple submissions.

Why this matters

Without proper loading state management and submission prevention, users could trigger multiple pool creation requests, potentially causing race conditions or duplicate transactions.

Suggested change ∙ Feature Preview

Implement loading state and disable button during submission:

const [isSubmitting, setIsSubmitting] = useState(false);

// In button handler:
onPress={async () => {
    if (isSubmitting) return;
    setIsSubmitting(true);
    setShowModal(true);
    try {
        const resp = await createPool();
        if (!resp) {
            setShowErrorModal(true);
        } else {
            setShowModal(false);
            nextStep();
        }
    } finally {
        setIsSubmitting(false);
    }
}}
disabled={isSubmitting}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +182 to +184
<Text fontSize="xs" textAlign="center">
{form.claimAmountPerWeek}G$
</Text>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect Value Reuse category Functionality

Tell me more
What is the issue?

The same form.claimAmountPerWeek value is incorrectly used for both 'Min Claim Amount' and 'Amount To Fund' fields, which are logically different values.

Why this matters

Using the same value for two different business concepts will lead to incorrect pool configuration and potential financial implications.

Suggested change ∙ Feature Preview

Use the correct form field for 'Amount To Fund':

<Text fontSize="xs" textAlign="center">
  {form.amountToFund}G$
</Text>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +131 to +137
<TextArea
width="full"
h={20}
placeholder="Text Area Placeholder"
autoCompleteType={undefined}
value={form.poolRecipients}
/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Mode Field Editability category Functionality

Tell me more
What is the issue?

The TextArea for pool recipients is not disabled in review mode, suggesting it could be editable when it should only be for display.

Why this matters

Users might attempt to edit the pool recipients in the review step, leading to confusion as changes won't be saved and breaking the review-only nature of this step.

Suggested change ∙ Feature Preview

Add isReadOnly prop to TextArea:

<TextArea
  width="full"
  h={20}
  value={form.poolRecipients}
  isReadOnly
/>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +103 to +105
<Text fontWeight="600" fontSize="lg">
{form.adminWalletAddress}
</Text>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsanitized Wallet Address Display category Security

Tell me more
What is the issue?

The admin wallet address is displayed without validation or sanitization, potentially enabling XSS attacks if the address contains malicious scripts.

Why this matters

Displaying unsanitized wallet addresses could allow attackers to inject malicious scripts that execute in users' browsers, potentially compromising their security.

Suggested change ∙ Feature Preview

Sanitize the wallet address before display:

<Text fontWeight="600" fontSize="lg">
  {sanitizeAddress(form.adminWalletAddress)}
</Text>
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +6 to +8
const Success = () => {
const { isDesktopView } = useScreenSize();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent Component Library Usage category Design

Tell me more
What is the issue?

The component mixes native-base components (VStack) with plain HTML elements (div, img) which breaks the consistency of the component library usage.

Why this matters

Mixing different component libraries and native HTML elements makes the codebase harder to maintain, style consistently, and reduces the benefits of using a component library.

Suggested change ∙ Feature Preview

Replace HTML elements with native-base equivalents:

const Success = () => {
  const { isDesktopView } = useScreenSize();
  return (
    <VStack>
      <VStack>
        <Box // instead of div
          backgroundImage={`url(${Mask})`}
          ...>
          <Image // instead of img
            source={SuccessIcon}
            alt="Success Icon"
            width={100}
          />
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Collaborator

@L03TJ3 L03TJ3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. mobile responsiveness (like buttons at the bottom of the page) have to be fixed

  2. the checkbox on the pool type selection screen is confusing. if I click on it, nothing happens

  3. I can upload a bigger logo then the provided 500x500. (console does show the error)

  4. Pool configuration screen. I cannot select 'new members can join' if I leave the recipient list empty. Why?

  5. pool recipients shows as required, which is not (validation also passes through if left empty

  6. manager fee can exceed 30%

  7. claim amount per week:

  • The amount of expected members is not reflected in the supportive tasks (hardcoded 10 I assume)
  • the slider can break with NaN and becomes unresponsive if sliding too much
  • max amount of members is also not reflected in the review screen
  1. locally I cannot seem to launch my pool.
    Did you test the flow yourself, and deployed a first dev pool?

  2. I should not be able to start the flow if I dont have a connected wallet. see screen 2 of the mobile flow in figma

Also, follow up on the AI reviews.
and make sure to sync with master branch

REACT_APP_CELO_EXPLORER: 'https://celoscan.io',
REACT_APP_SUPERFLUID_EXPLORER: 'https://app.superfluid.finance/stream/celo',
REACT_APP_NETWORK: 'development-celo',
REACT_APP_NETWORK: 'hardhat',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid network. it should be aligned with the available networks as defined in deployment_json.

For local testing you can use whatever. but for dev environment this should always be development-celo.
Revert

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Aug 15, 2025

@pbkompasz Hey, whats the status of this?
have you seen my review, is it being worked on?

@L03TJ3
Copy link
Collaborator

L03TJ3 commented Aug 19, 2025

Hey @pbkompasz this is the final reminder, please go over the feedback and review and give an update on progress.
else we would have to unassign you in line with our contribution guidelines
see section 'some general rules'

@L03TJ3 L03TJ3 mentioned this pull request Aug 28, 2025
11 tasks
@L03TJ3 L03TJ3 mentioned this pull request Sep 8, 2025
@L03TJ3 L03TJ3 closed this Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants