feat: support sablier airdrop proposal type#901
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end Sablier airdrop support: new create-proposal UI and schema, Merkle tree + IPFS flow, encoding and contract helpers for Sablier airdrop factories, a hook to parse airdrop creations, proposal UI to display/claim airdrops, related types/constants, icon, and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Form as Airdrop Form (UI)
participant ENS as ENS Resolver
participant Merkle as Merkle Tree Builder
participant IPFS as IPFS Upload
participant Encode as Encoding Utils
participant Treasury as Treasury/WETH Handler
participant Chain as Blockchain (RPC / Factory)
User->>Form: Fill campaign details & upload recipients
Form->>ENS: Resolve ENS names (admin/recipients)
ENS-->>Form: Resolved addresses / errors
Form->>Merkle: Build Merkle tree (leaves: recipient,amount)
Merkle-->>Form: Root, tree payload, leaf proofs
Form->>IPFS: Upload payload (tree + metadata)
IPFS-->>Form: CID
Form->>Encode: Compute createMerkleInstant/createMerkleLL calldata
Encode-->>Form: Calldata
alt Token is ETH and needs wrapping
Form->>Treasury: Prepare WETH deposit/transfer calldata
Treasury-->>Form: WETH calldata
end
Form->>Chain: Submit batched transactions: [factory create, token transfer (or WETH deposit+transfer)]
Chain-->>Form: Tx receipts (CID/logs)
Form->>User: Show deployment result (campaign address, CID)
sequenceDiagram
participant ProposalPage as Proposal Page
participant Hook as useAirdropData
participant Decoder as Call Decoder (ABI)
participant Events as Execution Receipt Logs
participant Contracts as On-chain Contracts (ERC20, Campaign)
participant UI as AirdropDetails / AirdropItem
participant IPFS as IPFS/Gateway
ProposalPage->>Hook: Request airdrop data for proposal
Hook->>Decoder: Decode proposal.targets/calldatas for createMerkle calls
Decoder-->>Hook: Parsed params (type, params, aggregate, recipientCount)
Hook->>Events: Fetch execution tx receipt logs (if executed)
Events-->>Hook: Find factory create events -> campaign address
Hook->>Contracts: Query token metadata & balances for campaign addresses
Contracts-->>Hook: Metadata & balances
Hook-->>ProposalPage: AirdropInstanceData[] (with campaignAddress)
ProposalPage->>UI: Render AirdropDetails with instances
UI->>IPFS: Fetch merkle payload by CID (if executed)
UI->>Contracts: Check hasClaimed & minFee & simulate claim
Contracts-->>UI: Claim status & fee
UI-->>User: Display eligibility and allow claim action
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.ts (1)
136-139: Consider adding.required()to ensure recipients array is always defined.The
recipientsarray has.min(1)but lacks.required(). While Yup arrays with.min(1)validate that at least one element exists when the array is provided,undefinedmay still pass initial validation depending on form initialization.♻️ Proposed fix
recipients: yup .array() .of(recipientSchema) - .min(1, 'At least one recipient is required.'), + .min(1, 'At least one recipient is required.') + .required('Recipients are required.'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.ts` around lines 136 - 139, The recipients schema currently uses yup.array().of(recipientSchema).min(1, ...) but may allow undefined; update the recipients validation to include .required() so the array itself must be present (e.g., recipients: yup.array().of(recipientSchema).required('Recipients are required').min(1, 'At least one recipient is required.')), keeping the existing recipientSchema and error messages.packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsx (2)
88-104: The balance mapping logic is correct but subtle.The iteration correctly aligns
balanceResultsindices with filtered airdrops by only incrementingresultIndexwhencampaignAddressexists. This works becausebalanceContractsapplies the same filter. Consider adding a brief comment to clarify this alignment for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsx` around lines 88 - 104, balancesByCampaign builds a map by iterating airdropsWithMetadata and consuming balanceResults in lockstep only for airdrops that have campaignAddress; add a short clarifying comment above the loop in the balancesByCampaign useMemo explaining that resultIndex is only incremented for airdrops with campaignAddress because balanceResults was produced from the same filtered list (see airdropsWithMetadata, balanceResults and the balanceContracts creation) so indices remain aligned—this will help future maintainers understand the subtle index coordination.
26-36: Consider handling invalid addresses gracefully.
getAddress()from viem throws an error if the input is not a valid address. Ifairdrop.tokencontains an invalid address string, this will cause a runtime error during theuseMemocomputation.🛡️ Proposed defensive fix
const uniqueTokenAddresses = useMemo( () => Array.from( new Set( airdrops - .map((airdrop) => getAddress(airdrop.token)) - .filter((address): address is Address => !!address) + .map((airdrop) => { + try { + return getAddress(airdrop.token) + } catch { + return null + } + }) + .filter((address): address is Address => address !== null) ) ), [airdrops] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsx` around lines 26 - 36, The useMemo that computes uniqueTokenAddresses calls getAddress(airdrop.token) directly which will throw on invalid input; wrap the address normalization in a safe validator (e.g., a small helper or try/catch inside the map) so invalid airdrop.token values are filtered out instead of throwing. Update the logic around uniqueTokenAddresses (and the map/filter chain using airdrops and getAddress) to attempt normalization inside a try/catch or use a validation utility before calling getAddress, and only include non-empty, successfully normalized Address values in the resulting Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.ts`:
- Around line 131-134: Replace the use of
addressValidationSchemaWithError(...).optional() for the tokenAddress field with
the provided addressValidationOptionalSchema to correctly make the address
optional; locate the tokenAddress declaration in AirdropTokens.schema (currently
using addressValidationSchemaWithError) and swap it to
addressValidationOptionalSchema (or wrap addressValidationOptionalSchema in a
.test() if a custom error message is required); also search for the same pattern
in StreamTokens, SendTokens, and MilestonePayments schemas and apply the same
replacement to avoid the .required() + .optional() mismatch.
In
`@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx`:
- Around line 196-199: When isEth is true the form still displays the native
token symbol even though transactions will use wrappedTokenAddress; update the
UI to show the wrapped asset symbol wherever tokenSymbol is used. Replace uses
of tokenSymbol (and any direct reads from values.tokenMetadata.symbol) with a
derived displaySymbol = isEth ? (lookupWrappedSymbolFrom(chain.id) or the known
wrapped symbol for the chain) : tokenSymbol, and use displaySymbol in the
totals, queued summary, and summary components referenced around the blocks
using tokenSymbol (also update the usages noted at 275-277, 385-389, 415-435) so
the form and summaries explicitly label ETH selections as WETH. Ensure the
underlying transaction still uses wrappedTokenAddress while only the displayed
symbol changes.
- Around line 543-549: The datetime inputs (e.g., the input bound to
formik.values.campaignStartDate) are currently using setFieldValue only and lack
name/onBlur, so touched never updates; replace the manual value/onChange wiring
with Formik's wiring by applying formik.getFieldProps('campaignStartDate') (or
at minimum add name="campaignStartDate" and onBlur={formik.handleBlur} alongside
setFieldValue) so blur lifecycle and validateOnBlur work; make the same change
for the other datetime inputs referenced (the inputs at the other ranges in this
component).
- Around line 75-89: initialValues.adminAddress is being derived from the async
escrowDelegate which causes Formik (with enableReinitialize=true) to reset the
whole form when escrowDelegate resolves; instead, seed adminAddress only once on
mount and update only that field via setFieldValue when escrowDelegate arrives.
Concretely: stop recreating the initialValues object from escrowDelegate after
mount (keep a stable initialValues for Formik/AirdropTokensValues), record
whether adminAddress was seeded (use a ref or local state), and in a useEffect
watching escrowDelegate call formik.setFieldValue('adminAddress', escrowDelegate
|| addresses.treasury) only if the adminAddress was not manually edited;
reference initialValues, adminAddress, escrowDelegate, addresses.treasury,
enableReinitialize, and setFieldValue to locate where to change the logic.
In `@packages/hooks/src/useAirdropData.ts`:
- Around line 113-132: The optional ERC20 funding-transfer probe (the
decodeFunctionData call using erc20Abi that populates fundingTransfer when
nextCalldata/nextTarget match params.token) must be isolated in its own
try/catch so a decode failure doesn’t abort the outer parsing/validation; wrap
the block that checks nextCalldata/nextTarget and calls decodeFunctionData (the
logic that sets fundingTransfer) in a separate try/catch, on error simply
skip/leave fundingTransfer undefined and optionally log a debug message, but do
not rethrow so the surrounding function/outer try/catch can continue processing
the valid campaign.
In
`@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx`:
- Around line 113-129: The code currently loads a Merkle tree from IPFS (via
fetchJsonFromCid and StandardMerkleTree.load) and uses tree.root directly; add a
guard that verifies the loaded tree.root equals the trusted airdrop.merkleRoot
(from the airdrop object in scope) and throw an error (or reject) if they differ
so stale/malicious IPFS payloads are rejected before building proofs or marking
wallets eligible. Locate the async loader that calls fetchJsonFromCid and
StandardMerkleTree.load, compare tree.root to airdrop.merkleRoot, and ensure you
surface a clear error (e.g., throw new Error('Merkle root mismatch')) when they
do not match. Ensure this check runs before returning { tree, root } and before
any proof generation logic that consumes tree.
---
Nitpick comments:
In
`@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.ts`:
- Around line 136-139: The recipients schema currently uses
yup.array().of(recipientSchema).min(1, ...) but may allow undefined; update the
recipients validation to include .required() so the array itself must be present
(e.g., recipients: yup.array().of(recipientSchema).required('Recipients are
required').min(1, 'At least one recipient is required.')), keeping the existing
recipientSchema and error messages.
In
`@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsx`:
- Around line 88-104: balancesByCampaign builds a map by iterating
airdropsWithMetadata and consuming balanceResults in lockstep only for airdrops
that have campaignAddress; add a short clarifying comment above the loop in the
balancesByCampaign useMemo explaining that resultIndex is only incremented for
airdrops with campaignAddress because balanceResults was produced from the same
filtered list (see airdropsWithMetadata, balanceResults and the balanceContracts
creation) so indices remain aligned—this will help future maintainers understand
the subtle index coordination.
- Around line 26-36: The useMemo that computes uniqueTokenAddresses calls
getAddress(airdrop.token) directly which will throw on invalid input; wrap the
address normalization in a safe validator (e.g., a small helper or try/catch
inside the map) so invalid airdrop.token values are filtered out instead of
throwing. Update the logic around uniqueTokenAddresses (and the map/filter chain
using airdrops and getAddress) to attempt normalization inside a try/catch or
use a validation utility before calling getAddress, and only include non-empty,
successfully normalized Address values in the resulting Set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4082cb39-92f1-4457-bb0e-1a3b807146b2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/constants/src/swrKeys.tspackages/create-proposal-ui/package.jsonpackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.tspackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsxpackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokensDetailsDisplay.tsxpackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/index.tspackages/create-proposal-ui/src/components/TransactionForm/TransactionForm.tsxpackages/hooks/src/index.tspackages/hooks/src/useAirdropData.tspackages/proposal-ui/package.jsonpackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsxpackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsxpackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/index.tspackages/proposal-ui/src/components/ProposalDescription/ProposalDescription.tsxpackages/proposal-ui/src/constants/transactionTypes.tspackages/types/src/transaction.tspackages/utils/src/sablier/constants.tspackages/utils/src/sablier/contracts.tspackages/utils/src/sablier/encoding.ts
...ages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.ts
Outdated
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx
Outdated
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx
Outdated
Show resolved
Hide resolved
packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/create-proposal-ui/src/components/TransactionForm/MilestonePayments/MilestonePaymentsForm.tsx (1)
30-47:formikin useEffect dependencies causes unnecessary effect execution on every render.The
formikobject fromuseFormikContextis a new reference on every render. While the conditional logic prevents infinite updates, the effect body still runs unnecessarily. Extract only the specific values and functions needed.♻️ Proposed refactor to avoid unnecessary effect runs
const SyncClientAddressFromEscrowDelegate = ({ escrowDelegate, treasury, }: { escrowDelegate?: string | null treasury?: string | null }) => { - const formik = useFormikContext<MilestonePaymentsFormValues>() + const { values, touched, setFieldValue } = useFormikContext<MilestonePaymentsFormValues>() + const currentClientAddress = values.clientAddress + const isClientAddressTouched = touched.clientAddress useEffect(() => { const preferred = escrowDelegate || treasury || '' if (!preferred) return - const current = String(formik.values.clientAddress || '') - const touched = Boolean(formik.touched.clientAddress) + const current = String(currentClientAddress || '') + const isTouched = Boolean(isClientAddressTouched) const manuallyEdited = current.length > 0 && current !== String(treasury || '') && current !== String(escrowDelegate || '') - if (!touched && !manuallyEdited && current !== preferred) { - formik.setFieldValue('clientAddress', preferred, false) + if (!isTouched && !manuallyEdited && current !== preferred) { + setFieldValue('clientAddress', preferred, false) } - }, [escrowDelegate, treasury, formik]) + }, [escrowDelegate, treasury, currentClientAddress, isClientAddressTouched, setFieldValue]) return null }Note:
setFieldValuefrom Formik is typically stable, but if you observe issues, you can wrap it in auseCallbackor use a ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/TransactionForm/MilestonePayments/MilestonePaymentsForm.tsx` around lines 30 - 47, The useEffect is depending on the whole formik object, causing unnecessary runs; destructure only the needed pieces from useFormikContext (clientAddress value, touched.clientAddress, and setFieldValue) and use those in the effect dependencies instead of formik so the effect only runs when those specific values change; locate the useFormikContext<MilestonePaymentsFormValues>() call and replace its single formik reference with destructured symbols (clientAddress, touchedClientAddress, setFieldValue) and update the effect to reference those symbols (and keep escrowDelegate and treasury in deps).packages/create-proposal-ui/src/components/TransactionForm/StreamTokens/StreamTokens.tsx (1)
60-77: Avoid including the entireformikobject in useEffect dependencies.
useFormikContext()returns a new object reference on every render. Includingformikin the dependency array causes this effect to run on every render rather than only whenescrowDelegateortreasuryAddresschange, leading to unnecessary updates and potential performance issues.Extract only the needed values and use refs to read current form state without triggering re-runs:
♻️ Proposed refactor using refs for stable dependencies
const SyncSenderAddressFromEscrowDelegate = ({ escrowDelegate, treasuryAddress, }: { escrowDelegate?: string | null treasuryAddress?: string | null }) => { - const formik = useFormikContext<StreamTokensValues>() + const { values, touched, setFieldValue } = useFormikContext<StreamTokensValues>() + const valuesRef = useRef(values) + valuesRef.current = values + const touchedRef = useRef(touched) + touchedRef.current = touched useEffect(() => { const preferred = escrowDelegate || treasuryAddress || '' if (!preferred) return - const current = String(formik.values.senderAddress || '') - const touched = Boolean(formik.touched.senderAddress) + const current = String(valuesRef.current.senderAddress || '') + const isTouched = Boolean(touchedRef.current.senderAddress) const manuallyEdited = current.length > 0 && current !== String(treasuryAddress || '') && current !== String(escrowDelegate || '') - if (!touched && !manuallyEdited && current !== preferred) { - formik.setFieldValue('senderAddress', preferred, false) + if (!isTouched && !manuallyEdited && current !== preferred) { + setFieldValue('senderAddress', preferred, false) } - }, [escrowDelegate, treasuryAddress, formik]) + }, [escrowDelegate, treasuryAddress, setFieldValue]) return null }Note: You'll need to add
useRefto the imports from React at line 27.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/TransactionForm/StreamTokens/StreamTokens.tsx` around lines 60 - 77, Formik's whole object is included in the useEffect deps causing constant re-runs; instead destructure only the needed pieces from useFormikContext (e.g., const { values: { senderAddress }, touched: { senderAddress: touchedSenderAddress }, setFieldValue } = useFormikContext()) and update the effect to read those destructured symbols (senderAddress, touchedSenderAddress, setFieldValue) and have a dependency array of [escrowDelegate, treasuryAddress, senderAddress, touchedSenderAddress, setFieldValue]; this removes formik from the deps while preserving correct behavior (or alternatively store a stable ref via useRef to read current form values inside the effect) and add useRef to React imports if you choose the ref approach.packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx (1)
451-469: Consider extracting duplicated validation logic.The validation and submission flow is duplicated between the desktop
ContractButtonhandler (lines 451-469) and the mobileonContinuehandler (lines 497-516). Consider extracting a shared handler function to reduce duplication.Suggested extraction
const handleValidateAndSubmit = useCallback(async () => { setHasAttemptedSubmit(true) const errors = await formik.validateForm() if (Object.keys(errors).length > 0) { formik.setTouched({ title: true, summary: true }, true) setError(undefined) return } setError(undefined) await formik.submitForm() }, [formik])Then use in both handlers:
-handleClick={async () => { - setHasAttemptedSubmit(true) - const errors = await formik.validateForm() - // ... rest -}} +handleClick={handleValidateAndSubmit}Also applies to: 497-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx` around lines 451 - 469, Extract the duplicated validation/submission logic used in the ContractButton click handler and the mobile onContinue handler into a single reusable async function (e.g., handleValidateAndSubmit) that calls setHasAttemptedSubmit(true), awaits formik.validateForm(), checks Object.keys(errors).length to set touched via formik.setTouched({ title: true, summary: true }, true) and setError(undefined) on failure, and otherwise calls setError(undefined) followed by await formik.submitForm(); replace the inline blocks in both the desktop ContractButton handler and the mobile onContinue handler with calls to this new function and wrap it with useCallback where appropriate to preserve stable identity.apps/web/src/pages/dao/[network]/[token]/proposal/create.tsx (1)
186-231: Duplicated validation logic betweenmissingDraftRequirementsandtitleError.The title validation logic is repeated: both functions check for empty title, regex format, and max length. If validation rules change, both locations must be updated, risking inconsistency.
Suggested approach - derive titleError first and use it
+ const titleError = useMemo(() => { + const normalizedTitle = title?.trim() || '' + if (!normalizedTitle) return PROPOSAL_TITLE_REQUIRED_ERROR + if (!PROPOSAL_TITLE_REGEX.test(normalizedTitle)) return PROPOSAL_TITLE_FORMAT_ERROR + if (normalizedTitle.length > PROPOSAL_TITLE_MAX_LENGTH) return PROPOSAL_TITLE_MAX_ERROR + return undefined + }, [title]) + + const summaryError = useMemo(() => { + const normalizedSummary = summary?.trim() || '' + if (!normalizedSummary) return PROPOSAL_SUMMARY_REQUIRED_ERROR + return undefined + }, [summary]) const missingDraftRequirements = useMemo(() => { const requirements: string[] = [] - const normalizedTitle = title?.trim() || '' - const normalizedSummary = summary?.trim() || '' - - if (!normalizedTitle) { - requirements.push('add a proposal title') - } else if (!PROPOSAL_TITLE_REGEX.test(normalizedTitle)) { - requirements.push('fix the proposal title format') - } else if (normalizedTitle.length > PROPOSAL_TITLE_MAX_LENGTH) { - requirements.push('shorten the proposal title') - } - - if (!normalizedSummary) { - requirements.push('add a proposal summary') - } + if (titleError === PROPOSAL_TITLE_REQUIRED_ERROR) requirements.push('add a proposal title') + else if (titleError === PROPOSAL_TITLE_FORMAT_ERROR) requirements.push('fix the proposal title format') + else if (titleError === PROPOSAL_TITLE_MAX_ERROR) requirements.push('shorten the proposal title') + + if (summaryError) requirements.push('add a proposal summary') return requirements - }, [title, summary]) + }, [titleError, summaryError])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx around lines 186 - 231, Compute normalizedTitle and titleError once (in the existing titleError useMemo) and reuse titleError inside missingDraftRequirements instead of repeating the checks; specifically, ensure titleError (derived using PROPOSAL_TITLE_REGEX and PROPOSAL_TITLE_MAX_LENGTH) is calculated first and then have missingDraftRequirements push the appropriate messages based on titleError values (e.g., map PROPOSAL_TITLE_REQUIRED_ERROR → 'add a proposal title', PROPOSAL_TITLE_FORMAT_ERROR → 'fix the proposal title format', PROPOSAL_TITLE_MAX_ERROR → 'shorten the proposal title'), and similarly reuse normalizedSummary/summaryError in missingDraftRequirements to avoid duplicating summary validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/create-proposal-ui/src/components/ReviewProposalForm/fields.ts`:
- Line 31: The Yup validation for the summary field currently chains .optional()
and .required(), which is contradictory; update the schema by removing the
.optional() call on the summary field so it reads as
Yup.string().required(PROPOSAL_SUMMARY_REQUIRED_ERROR) (locate the "summary"
field in the schema definition in fields.ts).
In
`@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx`:
- Around line 67-82: The effect is re-running because the whole formik object is
in the dependency array; destructure the specific bits you need from formik
(e.g., const { values: { adminAddress }, touched: { adminAddress: touchedAdmin
}, setFieldValue } = formik) and use those primitives in the useEffect deps
instead of formik, then update the effect to reference adminAddress,
touchedAdmin, setFieldValue, escrowDelegate and treasuryAddress so the effect
only runs when the actual values it depends on change.
In `@packages/create-proposal-ui/src/constants/proposalValidation.ts`:
- Around line 2-6: PROPOSAL_TITLE_MAX_LENGTH (currently 5000) and
PROPOSAL_TITLE_MAX_ERROR ("< 256 characters") are inconsistent; pick the
intended limit and make them match: either set PROPOSAL_TITLE_MAX_LENGTH = 256
to match PROPOSAL_TITLE_MAX_ERROR, or update PROPOSAL_TITLE_MAX_ERROR to reflect
"≤ 5000 characters" (or the chosen limit). Also search for usages of
PROPOSAL_TITLE_MAX_LENGTH (validation functions/components) and ensure any
user-facing messages or checks use the updated constant/value.
In
`@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx`:
- Around line 65-66: The toDateString helper renders chain timestamps using the
viewer's local timezone which can shift on-chain UTC dates; update the
toDateString(timestamp: number) implementation to format dates in a fixed
timezone (UTC) — e.g., create the Date from timestamp*1000 and call
toLocaleDateString with an options object including timeZone: 'UTC' (or
otherwise produce a UTC-based YYYY-MM-DD string) so the displayed
start/expiration dates match the on-chain window consistently.
- Around line 196-215: The component currently defaults claimReadResults to
false/0 which briefly marks airdrops as claimable; update the logic so
hasClaimed and minFeeWei are only derived when the corresponding
claimReadResults entries have status 'success' (i.e. treat unresolved reads as
unknown, not as false/0), and include the read-success check into the
isClaimable computation (reference hasClaimed, minFeeWei, claimReadResults,
eligibleLeaf, currentBalance, isClaimable) so that isClaimable is false until
reads succeed; additionally modify handleClaim to early-return (bail out) if the
minFeeWei read is not yet available/successful to avoid triggering a failing
wallet flow.
---
Nitpick comments:
In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx:
- Around line 186-231: Compute normalizedTitle and titleError once (in the
existing titleError useMemo) and reuse titleError inside
missingDraftRequirements instead of repeating the checks; specifically, ensure
titleError (derived using PROPOSAL_TITLE_REGEX and PROPOSAL_TITLE_MAX_LENGTH) is
calculated first and then have missingDraftRequirements push the appropriate
messages based on titleError values (e.g., map PROPOSAL_TITLE_REQUIRED_ERROR →
'add a proposal title', PROPOSAL_TITLE_FORMAT_ERROR → 'fix the proposal title
format', PROPOSAL_TITLE_MAX_ERROR → 'shorten the proposal title'), and similarly
reuse normalizedSummary/summaryError in missingDraftRequirements to avoid
duplicating summary validation logic.
In
`@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx`:
- Around line 451-469: Extract the duplicated validation/submission logic used
in the ContractButton click handler and the mobile onContinue handler into a
single reusable async function (e.g., handleValidateAndSubmit) that calls
setHasAttemptedSubmit(true), awaits formik.validateForm(), checks
Object.keys(errors).length to set touched via formik.setTouched({ title: true,
summary: true }, true) and setError(undefined) on failure, and otherwise calls
setError(undefined) followed by await formik.submitForm(); replace the inline
blocks in both the desktop ContractButton handler and the mobile onContinue
handler with calls to this new function and wrap it with useCallback where
appropriate to preserve stable identity.
In
`@packages/create-proposal-ui/src/components/TransactionForm/MilestonePayments/MilestonePaymentsForm.tsx`:
- Around line 30-47: The useEffect is depending on the whole formik object,
causing unnecessary runs; destructure only the needed pieces from
useFormikContext (clientAddress value, touched.clientAddress, and setFieldValue)
and use those in the effect dependencies instead of formik so the effect only
runs when those specific values change; locate the
useFormikContext<MilestonePaymentsFormValues>() call and replace its single
formik reference with destructured symbols (clientAddress, touchedClientAddress,
setFieldValue) and update the effect to reference those symbols (and keep
escrowDelegate and treasury in deps).
In
`@packages/create-proposal-ui/src/components/TransactionForm/StreamTokens/StreamTokens.tsx`:
- Around line 60-77: Formik's whole object is included in the useEffect deps
causing constant re-runs; instead destructure only the needed pieces from
useFormikContext (e.g., const { values: { senderAddress }, touched: {
senderAddress: touchedSenderAddress }, setFieldValue } = useFormikContext()) and
update the effect to read those destructured symbols (senderAddress,
touchedSenderAddress, setFieldValue) and have a dependency array of
[escrowDelegate, treasuryAddress, senderAddress, touchedSenderAddress,
setFieldValue]; this removes formik from the deps while preserving correct
behavior (or alternatively store a stable ref via useRef to read current form
values inside the effect) and add useRef to React imports if you choose the ref
approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19312c06-8846-4839-bbc6-198b2b13bcc3
📒 Files selected for processing (17)
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsxapps/web/src/pages/dao/[network]/[token]/proposal/review.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/fields.tspackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.schema.tspackages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsxpackages/create-proposal-ui/src/components/TransactionForm/MilestonePayments/MilestonePayments.schema.tspackages/create-proposal-ui/src/components/TransactionForm/MilestonePayments/MilestonePaymentsForm.tsxpackages/create-proposal-ui/src/components/TransactionForm/SendTokens/SendTokens.schema.tspackages/create-proposal-ui/src/components/TransactionForm/StreamTokens/StreamTokens.schema.tspackages/create-proposal-ui/src/components/TransactionForm/StreamTokens/StreamTokens.tsxpackages/create-proposal-ui/src/constants/index.tspackages/create-proposal-ui/src/constants/proposalValidation.tspackages/create-proposal-ui/src/index.tspackages/hooks/src/useAirdropData.tspackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsxpackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/hooks/src/useAirdropData.ts
- packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsx
| .required(PROPOSAL_TITLE_REQUIRED_ERROR) | ||
| .matches(PROPOSAL_TITLE_REGEX, PROPOSAL_TITLE_FORMAT_ERROR) | ||
| .max(PROPOSAL_TITLE_MAX_LENGTH, PROPOSAL_TITLE_MAX_ERROR), | ||
| summary: Yup.string().optional().required(PROPOSAL_SUMMARY_REQUIRED_ERROR), |
Contributor
There was a problem hiding this comment.
Remove contradictory .optional() from summary validation.
Chaining .optional().required() is contradictory—.required() overrides .optional(), making the .optional() call redundant and the intent unclear. Since the error constant indicates summary is required, remove .optional().
Proposed fix
- summary: Yup.string().optional().required(PROPOSAL_SUMMARY_REQUIRED_ERROR),
+ summary: Yup.string().required(PROPOSAL_SUMMARY_REQUIRED_ERROR),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| summary: Yup.string().optional().required(PROPOSAL_SUMMARY_REQUIRED_ERROR), | |
| summary: Yup.string().required(PROPOSAL_SUMMARY_REQUIRED_ERROR), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-proposal-ui/src/components/ReviewProposalForm/fields.ts` at
line 31, The Yup validation for the summary field currently chains .optional()
and .required(), which is contradictory; update the schema by removing the
.optional() call on the summary field so it reads as
Yup.string().required(PROPOSAL_SUMMARY_REQUIRED_ERROR) (locate the "summary"
field in the schema definition in fields.ts).
Comment on lines
+67
to
+82
| useEffect(() => { | ||
| const preferred = escrowDelegate || treasuryAddress || '' | ||
| if (!preferred) return | ||
|
|
||
| const current = String(formik.values.adminAddress || '') | ||
| const touched = Boolean(formik.touched.adminAddress) | ||
|
|
||
| const manuallyEdited = | ||
| current.length > 0 && | ||
| current !== String(treasuryAddress || '') && | ||
| current !== String(escrowDelegate || '') | ||
|
|
||
| if (!touched && !manuallyEdited && current !== preferred) { | ||
| formik.setFieldValue('adminAddress', preferred, false) | ||
| } | ||
| }, [escrowDelegate, treasuryAddress, formik]) |
Contributor
There was a problem hiding this comment.
Excessive re-renders due to formik object in dependency array.
The formik object reference changes on every render, causing this effect to run unnecessarily on each form update. Extract the specific values needed from formik before the effect, or use useFormikContext values directly in the dependency array.
Proposed fix
const SyncAdminAddressFromEscrowDelegate = ({
escrowDelegate,
treasuryAddress,
}: {
escrowDelegate?: string | null
treasuryAddress?: string | null
}) => {
const formik = useFormikContext<AirdropTokensValues>()
+ const adminAddress = formik.values.adminAddress
+ const touched = formik.touched.adminAddress
+ const setFieldValue = formik.setFieldValue
useEffect(() => {
const preferred = escrowDelegate || treasuryAddress || ''
if (!preferred) return
- const current = String(formik.values.adminAddress || '')
- const touched = Boolean(formik.touched.adminAddress)
+ const current = String(adminAddress || '')
+ const isTouched = Boolean(touched)
const manuallyEdited =
current.length > 0 &&
current !== String(treasuryAddress || '') &&
current !== String(escrowDelegate || '')
- if (!touched && !manuallyEdited && current !== preferred) {
- formik.setFieldValue('adminAddress', preferred, false)
+ if (!isTouched && !manuallyEdited && current !== preferred) {
+ setFieldValue('adminAddress', preferred, false)
}
- }, [escrowDelegate, treasuryAddress, formik])
+ }, [escrowDelegate, treasuryAddress, adminAddress, touched, setFieldValue])
return null
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/create-proposal-ui/src/components/TransactionForm/AirdropTokens/AirdropTokens.tsx`
around lines 67 - 82, The effect is re-running because the whole formik object
is in the dependency array; destructure the specific bits you need from formik
(e.g., const { values: { adminAddress }, touched: { adminAddress: touchedAdmin
}, setFieldValue } = formik) and use those primitives in the useEffect deps
instead of formik, then update the effect to reference adminAddress,
touchedAdmin, setFieldValue, escrowDelegate and treasuryAddress so the effect
only runs when the actual values it depends on change.
Comment on lines
+2
to
+6
| export const PROPOSAL_TITLE_MAX_LENGTH = 5000 | ||
|
|
||
| export const PROPOSAL_TITLE_REQUIRED_ERROR = 'Proposal title is required' | ||
| export const PROPOSAL_TITLE_FORMAT_ERROR = 'only numbers or letters' | ||
| export const PROPOSAL_TITLE_MAX_ERROR = '< 256 characters' |
Contributor
There was a problem hiding this comment.
Mismatch between max length constant and error message.
PROPOSAL_TITLE_MAX_LENGTH is set to 5000, but PROPOSAL_TITLE_MAX_ERROR states '< 256 characters'. This inconsistency will confuse users when validation fails.
Proposed fix - align the values
Either reduce the max length to match the error:
-export const PROPOSAL_TITLE_MAX_LENGTH = 5000
+export const PROPOSAL_TITLE_MAX_LENGTH = 256Or update the error message to match the constant:
-export const PROPOSAL_TITLE_MAX_ERROR = '< 256 characters'
+export const PROPOSAL_TITLE_MAX_ERROR = '< 5000 characters'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const PROPOSAL_TITLE_MAX_LENGTH = 5000 | |
| export const PROPOSAL_TITLE_REQUIRED_ERROR = 'Proposal title is required' | |
| export const PROPOSAL_TITLE_FORMAT_ERROR = 'only numbers or letters' | |
| export const PROPOSAL_TITLE_MAX_ERROR = '< 256 characters' | |
| export const PROPOSAL_TITLE_MAX_LENGTH = 256 | |
| export const PROPOSAL_TITLE_REQUIRED_ERROR = 'Proposal title is required' | |
| export const PROPOSAL_TITLE_FORMAT_ERROR = 'only numbers or letters' | |
| export const PROPOSAL_TITLE_MAX_ERROR = '< 256 characters' |
Suggested change
| export const PROPOSAL_TITLE_MAX_LENGTH = 5000 | |
| export const PROPOSAL_TITLE_REQUIRED_ERROR = 'Proposal title is required' | |
| export const PROPOSAL_TITLE_FORMAT_ERROR = 'only numbers or letters' | |
| export const PROPOSAL_TITLE_MAX_ERROR = '< 256 characters' | |
| export const PROPOSAL_TITLE_MAX_LENGTH = 5000 | |
| export const PROPOSAL_TITLE_REQUIRED_ERROR = 'Proposal title is required' | |
| export const PROPOSAL_TITLE_FORMAT_ERROR = 'only numbers or letters' | |
| export const PROPOSAL_TITLE_MAX_ERROR = '< 5000 characters' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/create-proposal-ui/src/constants/proposalValidation.ts` around lines
2 - 6, PROPOSAL_TITLE_MAX_LENGTH (currently 5000) and PROPOSAL_TITLE_MAX_ERROR
("< 256 characters") are inconsistent; pick the intended limit and make them
match: either set PROPOSAL_TITLE_MAX_LENGTH = 256 to match
PROPOSAL_TITLE_MAX_ERROR, or update PROPOSAL_TITLE_MAX_ERROR to reflect "≤ 5000
characters" (or the chosen limit). Also search for usages of
PROPOSAL_TITLE_MAX_LENGTH (validation functions/components) and ensure any
user-facing messages or checks use the updated constant/value.
Comment on lines
+65
to
+66
| const toDateString = (timestamp: number): string => | ||
| new Date(timestamp * 1000).toLocaleDateString('en-US') |
Contributor
There was a problem hiding this comment.
Render chain timestamps in a fixed timezone.
toLocaleDateString('en-US') uses the viewer’s local timezone, so a UTC timestamp near midnight can display as the previous/next day depending on where the wallet is opened. That makes the shown start/expiration dates inconsistent with the actual on-chain window.
Suggested fix
const toDateString = (timestamp: number): string =>
- new Date(timestamp * 1000).toLocaleDateString('en-US')
+ new Date(timestamp * 1000).toLocaleDateString('en-US', {
+ timeZone: 'UTC',
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const toDateString = (timestamp: number): string => | |
| new Date(timestamp * 1000).toLocaleDateString('en-US') | |
| const toDateString = (timestamp: number): string => | |
| new Date(timestamp * 1000).toLocaleDateString('en-US', { | |
| timeZone: 'UTC', | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx`
around lines 65 - 66, The toDateString helper renders chain timestamps using the
viewer's local timezone which can shift on-chain UTC dates; update the
toDateString(timestamp: number) implementation to format dates in a fixed
timezone (UTC) — e.g., create the Date from timestamp*1000 and call
toLocaleDateString with an options object including timeZone: 'UTC' (or
otherwise produce a UTC-based YYYY-MM-DD string) so the displayed
start/expiration dates match the on-chain window consistently.
Comment on lines
+196
to
+215
| const hasClaimed = | ||
| claimReadResults?.[0]?.status === 'success' | ||
| ? Boolean(claimReadResults[0].result) | ||
| : false | ||
|
|
||
| const minFeeWei = | ||
| claimReadResults?.[1]?.status === 'success' | ||
| ? (claimReadResults[1].result as bigint) | ||
| : 0n | ||
|
|
||
| const now = Math.floor(Date.now() / 1000) | ||
| const notStarted = airdrop.campaignStartTime > now | ||
| const expired = airdrop.expiration > 0 && airdrop.expiration <= now | ||
| const hasBalanceForClaim = | ||
| currentBalance !== undefined && eligibleLeaf | ||
| ? currentBalance >= eligibleLeaf.amount | ||
| : false | ||
|
|
||
| const isClaimable = | ||
| !!eligibleLeaf && !hasClaimed && !notStarted && !expired && hasBalanceForClaim |
Contributor
There was a problem hiding this comment.
Don’t default unresolved claim state to “claimable”.
Before the contract reads return, hasClaimed is forced to false and minFeeWei to 0n. That briefly marks the airdrop as claimable and enables the button even for already-claimed entries or campaigns that require a fee, which can send users into a failing wallet flow. Gate isClaimable on successful reads instead of optimistic fallbacks.
Suggested fix
const hasClaimed =
claimReadResults?.[0]?.status === 'success'
? Boolean(claimReadResults[0].result)
- : false
+ : undefined
const minFeeWei =
claimReadResults?.[1]?.status === 'success'
? (claimReadResults[1].result as bigint)
- : 0n
+ : undefined
const isClaimable =
- !!eligibleLeaf && !hasClaimed && !notStarted && !expired && hasBalanceForClaim
+ !!eligibleLeaf &&
+ hasClaimed === false &&
+ minFeeWei !== undefined &&
+ !notStarted &&
+ !expired &&
+ hasBalanceForClaimhandleClaim should also bail out until minFeeWei is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsx`
around lines 196 - 215, The component currently defaults claimReadResults to
false/0 which briefly marks airdrops as claimable; update the logic so
hasClaimed and minFeeWei are only derived when the corresponding
claimReadResults entries have status 'success' (i.e. treat unresolved reads as
unknown, not as false/0), and include the read-success check into the
isClaimable computation (reference hasClaimed, minFeeWei, claimReadResults,
eligibleLeaf, currentBalance, isClaimable) so that isClaimable is false until
reads succeed; additionally modify handleClaim to early-return (bail out) if the
minFeeWei read is not yet available/successful to avoid triggering a failing
wallet flow.
dan13ram
force-pushed
the
sablier-airdrops
branch
from
c589bfb to
09a3e38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx (2)
497-523: Consider extracting duplicated validation logic.The validation logic in
onContinue(lines 499-515) is nearly identical to thehandleClickhandler (lines 452-468). This duplication could lead to divergence during future maintenance.Consider extracting a shared validation handler:
♻️ Suggested refactor to reduce duplication
+ const handleValidateAndSubmit = React.useCallback( + async (formik: FormikProps<FormValues>) => { + setHasAttemptedSubmit(true) + const errors = await formik.validateForm() + + if (Object.keys(errors).length > 0) { + formik.setTouched({ title: true, summary: true }, true) + setError(undefined) + return + } + + setError(undefined) + await formik.submitForm() + }, + [] + )Then use it in both places:
handleClick={async () => { - setHasAttemptedSubmit(true) - const errors = await formik.validateForm() - ... + await handleValidateAndSubmit(formik) }}onContinue={() => { - setHasAttemptedSubmit(true) - void (async () => { - ... - })() + void handleValidateAndSubmit(formik) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx` around lines 497 - 523, Extract the duplicated validation+submit logic into a single async helper (e.g., validateAndSubmit) and call it from both the onContinue handler and handleClick: move the formik.validateForm + touching fields + setHasAttemptedSubmit + setError + formik.submitForm sequence into that helper, keep the same behavior (mark title/summary touched, setError(undefined) on success, return early on validation errors) and update onContinue and handleClick to invoke validateAndSubmit (and await it where needed); reference functions/variables formik.validateForm, formik.setTouched, formik.submitForm, setHasAttemptedSubmit, setError, and the onContinue/handleClick handlers when making the change.
428-436: Error display handles current schema but may break with nested validation errors.The
String(value)conversion works for the current flat validation schema, but iftransactionsvalidation ever returns nested field errors (e.g., per-transaction validation), this would render as[object Object]. Consider adding a type guard or recursive flattening if nested validation is planned.For the current use case, this implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx` around lines 428 - 436, The current error rendering in ReviewProposalForm uses String(value) on Object.entries(formik.errors) which will become "[object Object]" if nested validation (e.g., per-transaction errors under transactions) appears; update the rendering to first normalize/flatten error values into strings (for example add a small helper like flattenErrors or a type-check: if typeof value === 'string' use it, if Array join(', '), if object recursively collect nested string messages) and then map those flattened strings to Text components; reference the hasAttemptedSubmit check and formik.errors (and the transactions nested key) when implementing the helper so nested Formik errors are safely rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx:
- Around line 192-197: The transactions step currently only explains missing
title/summary/transactions while new draft validators (normalizedTitle,
PROPOSAL_TITLE_REGEX, PROPOSAL_TITLE_MAX_LENGTH) can block canContinueToReview
without visible reason; update the transactions-step helper copy to derive its
message from the same validation state used to compute titleError and
missingDraftRequirements so users see formatted/too-long title errors and get a
“Go to Write Proposal” shortcut when blocked. Locate where the transactions
helper text is rendered (the transactions step component in
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsx), replace the
static helper copy with logic that reads titleError and missingDraftRequirements
(or reuses the normalizedTitle validation) to build the helper string and render
a button/link that navigates to the write proposal stage when those
title-related blockers exist, ensuring canContinueToReview failure reasons are
surfaced.
In `@packages/create-proposal-ui/src/components/ReviewProposalForm/fields.ts`:
- Around line 27-31: The title and summary validators in the shared Yup schema
should trim input before other checks so whitespace-only or outer-whitespace
values behave the same as the draft step; update the title and summary schema
entries in fields.ts (the title and summary Yup.string() chains) to normalize
strings (e.g., add .trim() or a .transform(v => typeof v === 'string' ? v.trim()
: v) at the start of each chain) before calling .required(), .matches(), .max(),
etc., leaving the existing PROPOSAL_TITLE_* and PROPOSAL_SUMMARY_* checks
intact.
---
Nitpick comments:
In
`@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx`:
- Around line 497-523: Extract the duplicated validation+submit logic into a
single async helper (e.g., validateAndSubmit) and call it from both the
onContinue handler and handleClick: move the formik.validateForm + touching
fields + setHasAttemptedSubmit + setError + formik.submitForm sequence into that
helper, keep the same behavior (mark title/summary touched, setError(undefined)
on success, return early on validation errors) and update onContinue and
handleClick to invoke validateAndSubmit (and await it where needed); reference
functions/variables formik.validateForm, formik.setTouched, formik.submitForm,
setHasAttemptedSubmit, setError, and the onContinue/handleClick handlers when
making the change.
- Around line 428-436: The current error rendering in ReviewProposalForm uses
String(value) on Object.entries(formik.errors) which will become "[object
Object]" if nested validation (e.g., per-transaction errors under transactions)
appears; update the rendering to first normalize/flatten error values into
strings (for example add a small helper like flattenErrors or a type-check: if
typeof value === 'string' use it, if Array join(', '), if object recursively
collect nested string messages) and then map those flattened strings to Text
components; reference the hasAttemptedSubmit check and formik.errors (and the
transactions nested key) when implementing the helper so nested Formik errors
are safely rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d2f866b-ecfc-4038-ba5f-90991a7a5625
📒 Files selected for processing (6)
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/fields.tspackages/create-proposal-ui/src/constants/index.tspackages/create-proposal-ui/src/constants/proposalValidation.tspackages/create-proposal-ui/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/create-proposal-ui/src/index.ts
- packages/create-proposal-ui/src/constants/proposalValidation.ts
packages/create-proposal-ui/src/components/ReviewProposalForm/fields.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsx (2)
207-231: Consider adding "touched" state for better UX.Currently,
titleErrorandsummaryErrorare displayed immediately on mount, showing validation errors before the user has interacted with the fields. While this is consistent with the existingmissingDraftRequirementsbehavior, it may feel aggressive to users seeing errors on a blank form.If you want to improve UX, you could track whether each field has been touched (blurred at least once) and only show errors after interaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx around lines 207 - 231, Add "touched" state for title and summary and only surface validation errors after the user interacts: create two boolean states (e.g., titleTouched and summaryTouched), set them true on the respective input onBlur handlers, then update the titleError and summaryError useMemo blocks to return errors only when the field is touched (e.g., if (!normalizedTitle) return PROPOSAL_TITLE_REQUIRED_ERROR only if titleTouched) while keeping the existing validation checks (PROPOSAL_TITLE_REGEX, PROPOSAL_TITLE_MAX_LENGTH) and preserving current dependencies ([title, titleTouched] and [summary, summaryTouched]); this ensures the inputs show errors only after blur.
186-231: Consider extracting shared validation logic to reduce duplication.The title normalization and validation checks (empty, regex, max length) are duplicated between
missingDraftRequirementsandtitleError. Similarly, summary normalization appears in bothmissingDraftRequirementsandsummaryError. If the validation rules change, both places need updating.♻️ Suggested refactor to consolidate validation
+ const normalizedTitle = useMemo(() => title?.trim() || '', [title]) + const normalizedSummary = useMemo(() => summary?.trim() || '', [summary]) + + const titleError = useMemo(() => { + if (!normalizedTitle) { + return PROPOSAL_TITLE_REQUIRED_ERROR + } + if (!PROPOSAL_TITLE_REGEX.test(normalizedTitle)) { + return PROPOSAL_TITLE_FORMAT_ERROR + } + if (normalizedTitle.length > PROPOSAL_TITLE_MAX_LENGTH) { + return PROPOSAL_TITLE_MAX_ERROR + } + return undefined + }, [normalizedTitle]) + + const summaryError = useMemo(() => { + if (!normalizedSummary) { + return PROPOSAL_SUMMARY_REQUIRED_ERROR + } + return undefined + }, [normalizedSummary]) + const missingDraftRequirements = useMemo(() => { const requirements: string[] = [] - - const normalizedTitle = title?.trim() || '' - const normalizedSummary = summary?.trim() || '' - - if (!normalizedTitle) { + if (!normalizedTitle) { requirements.push('add a proposal title') - } else if (!PROPOSAL_TITLE_REGEX.test(normalizedTitle)) { + } else if (!PROPOSAL_TITLE_REGEX.test(normalizedTitle)) { requirements.push('fix the proposal title format') - } else if (normalizedTitle.length > PROPOSAL_TITLE_MAX_LENGTH) { + } else if (normalizedTitle.length > PROPOSAL_TITLE_MAX_LENGTH) { requirements.push('shorten the proposal title') } - - if (!normalizedSummary) { + if (!normalizedSummary) { requirements.push('add a proposal summary') } - return requirements - }, [title, summary]) - - const titleError = useMemo(() => { - const normalizedTitle = title?.trim() || '' - ... - }, [title]) - - const summaryError = useMemo(() => { - const normalizedSummary = summary?.trim() || '' - ... - }, [summary]) + }, [normalizedTitle, normalizedSummary])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx around lines 186 - 231, Extract shared validation helpers (e.g., normalizeTitle, validateTitle, normalizeSummary, validateSummary) and use them inside missingDraftRequirements, titleError, and summaryError to remove duplicated logic: move trimming and checks against PROPOSAL_TITLE_REGEX and PROPOSAL_TITLE_MAX_LENGTH into validateTitle which returns an enum/string for empty/format/too-long or undefined, and move summary trimming into normalizeSummary/validateSummary that returns PROPOSAL_SUMMARY_REQUIRED_ERROR or undefined; then replace the inline checks in missingDraftRequirements, titleError, and summaryError to call these helpers and map their results to the same requirement strings and error constants (PROPOSAL_TITLE_REQUIRED_ERROR, PROPOSAL_TITLE_FORMAT_ERROR, PROPOSAL_TITLE_MAX_ERROR, PROPOSAL_SUMMARY_REQUIRED_ERROR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx`:
- Around line 488-494: The submit button's disabled prop currently blocks clicks
when title/summary/transactions are empty, preventing validateAndSubmit from
running and hasAttemptedSubmit from toggling; remove the content-validation
checks from the disabled expression (leave only in-flight states like simulating
or proposing and/or formik.isSubmitting) so Formik's validate/validateAndSubmit
runs and required-field errors can display; update the same logic for the other
submit button block referenced (the similar expression around lines 526-532) and
ensure validateAndSubmit and formik.submit handling remain unchanged.
- Around line 455-468: The current clickable Flex used for the skip-simulation
acknowledgement is not keyboard- or screen-reader-accessible; replace the
visual-only control with a real checkbox input (or a visually-styled <label>
containing an <input type="checkbox">) wired to skipSimulation and
setSkipSimulation so changes come from the input's onChange, and keep the same
visual styles by applying checkboxStyleVariants to the label/wrapper; ensure the
helper text (checkboxHelperText) is associated via aria-describedby or by
nesting the input in the label and that the Icon remains as the visual check
indicator only, so keyboard focus, space/enter activation, and screen-reader
semantics work correctly (update any event handlers that currently call
setSkipSimulation on Flex to use the input's onChange instead).
- Around line 369-375: The title/summary change handlers (onTitleChange,
onSummaryChange) update state with formik.setFieldValue and setTitle/setSummary
but do not clear or revalidate formik.errors because validateOnChange is off;
after calling formik.setFieldValue in both handlers, call
formik.validateField('title') and formik.validateField('summary') respectively
(or use formik.setFieldError('title','') / setFieldError('summary','') if you
prefer to just clear the error) so the error state is refreshed immediately when
the user edits the fields.
- Line 340: The native form submit is wired to formik.handleSubmit which creates
a secondary submit path that bypasses setHasAttemptedSubmit and the
ContractButton wallet/network guard; change the form's onSubmit to route into
the existing validateAndSubmit flow instead of formik.handleSubmit (remove
formik.handleSubmit usage on the <form>), making sure the handler calls
setHasAttemptedSubmit (or that validateAndSubmit itself does) and then invokes
the same checks that ContractButton enforces so Enter-key submissions follow the
same validation and wallet/network guard as the explicit submit buttons in
ProposalDraftForm.
---
Nitpick comments:
In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx:
- Around line 207-231: Add "touched" state for title and summary and only
surface validation errors after the user interacts: create two boolean states
(e.g., titleTouched and summaryTouched), set them true on the respective input
onBlur handlers, then update the titleError and summaryError useMemo blocks to
return errors only when the field is touched (e.g., if (!normalizedTitle) return
PROPOSAL_TITLE_REQUIRED_ERROR only if titleTouched) while keeping the existing
validation checks (PROPOSAL_TITLE_REGEX, PROPOSAL_TITLE_MAX_LENGTH) and
preserving current dependencies ([title, titleTouched] and [summary,
summaryTouched]); this ensures the inputs show errors only after blur.
- Around line 186-231: Extract shared validation helpers (e.g., normalizeTitle,
validateTitle, normalizeSummary, validateSummary) and use them inside
missingDraftRequirements, titleError, and summaryError to remove duplicated
logic: move trimming and checks against PROPOSAL_TITLE_REGEX and
PROPOSAL_TITLE_MAX_LENGTH into validateTitle which returns an enum/string for
empty/format/too-long or undefined, and move summary trimming into
normalizeSummary/validateSummary that returns PROPOSAL_SUMMARY_REQUIRED_ERROR or
undefined; then replace the inline checks in missingDraftRequirements,
titleError, and summaryError to call these helpers and map their results to the
same requirement strings and error constants (PROPOSAL_TITLE_REQUIRED_ERROR,
PROPOSAL_TITLE_FORMAT_ERROR, PROPOSAL_TITLE_MAX_ERROR,
PROPOSAL_SUMMARY_REQUIRED_ERROR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 308560f1-a306-4e41-aaf4-ed808eb905df
⛔ Files ignored due to path filters (1)
packages/zord/src/assets/airdrop-sablier.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsxpackages/create-proposal-ui/src/components/ReviewProposalForm/fields.tspackages/proposal-ui/src/constants/transactionTypes.tspackages/zord/src/icons.ts
packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx
Outdated
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx
Outdated
Show resolved
Hide resolved
packages/create-proposal-ui/src/components/ReviewProposalForm/ReviewProposalForm.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/create-proposal-ui/src/components/CreateProposalHeading/CreateProposalHeading.tsx (1)
69-79: Consider using constants and theme-aware styling.Two minor suggestions for improved maintainability:
The z-index
30matchesHERO_CONTENT_LAYERfrom@buildeross/constants. Consider importing and using the constant for consistency.The border color
rgba(0, 0, 0, 0.06)is hardcoded and may not adapt well to dark themes. Consider using a theme token if dark mode support is needed.♻️ Proposed fix for z-index constant
+import { HERO_CONTENT_LAYER } from '@buildeross/constants' // ... in style object: - zIndex: 30, + zIndex: HERO_CONTENT_LAYER,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-proposal-ui/src/components/CreateProposalHeading/CreateProposalHeading.tsx` around lines 69 - 79, Replace the hardcoded z-index and border color in CreateProposalHeading's Box: import HERO_CONTENT_LAYER from `@buildeross/constants` and use it instead of 30 (e.g., zIndex: HERO_CONTENT_LAYER), and swap the hardcoded 'rgba(0, 0, 0, 0.06)' for a theme-aware token (e.g., use theme.border or theme.colors.separator / a token like tokens.colors.borderSubtle) so the border adapts to dark mode; update the Box style block where stickyTopOffset and transition are set to reference these constants/tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/pages/dao/`[network]/[token]/proposal/create.tsx:
- Around line 450-460: The draft-shortcut is currently a non-interactive Text
with an onClick handler (see createStage, hasDraftBlockers, hasTitleDraftBlocker
and setCreateStage) which is not keyboard- or screen-reader accessible; replace
the Text with a proper interactive control (e.g., your Button component or a
link-styled Button) that preserves the same styling and click behavior, ensure
it supports keyboard focus and activation (Enter/Space) and uses the same
conditional label logic ('Fix title in Write Proposal' vs 'Go to Write
Proposal'), and keep the onClick to call setCreateStage('draft').
In
`@packages/create-proposal-ui/src/components/ProposalDraftForm/ProposalDraftForm.tsx`:
- Around line 25-26: The local touched flags titleTouched and summaryTouched in
ProposalDraftForm remain true when a parent clears or replaces the draft while
the form stays mounted, causing immediate validation errors; fix this by
resetting those flags whenever the draft/reset signal changes — for example, add
a useEffect in ProposalDraftForm that watches the draft prop (or a dedicated
remount/reset prop like draftId or resetCounter) and calls
setTitleTouched(false) and setSummaryTouched(false) when that prop changes so
the form returns to untouched state on a clear/replacement.
---
Nitpick comments:
In
`@packages/create-proposal-ui/src/components/CreateProposalHeading/CreateProposalHeading.tsx`:
- Around line 69-79: Replace the hardcoded z-index and border color in
CreateProposalHeading's Box: import HERO_CONTENT_LAYER from
`@buildeross/constants` and use it instead of 30 (e.g., zIndex:
HERO_CONTENT_LAYER), and swap the hardcoded 'rgba(0, 0, 0, 0.06)' for a
theme-aware token (e.g., use theme.border or theme.colors.separator / a token
like tokens.colors.borderSubtle) so the border adapts to dark mode; update the
Box style block where stickyTopOffset and transition are set to reference these
constants/tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc72259b-1a43-422d-9bb7-6bf80a7c9757
📒 Files selected for processing (9)
apps/web/src/pages/dao/[network]/[token]/proposal/create.tsxpackages/create-proposal-ui/src/components/CreateProposalHeading/CreateProposalHeading.tsxpackages/create-proposal-ui/src/components/ProposalDraftForm/ProposalDraftForm.tsxpackages/hooks/src/useAirdropData.tspackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropDetails.tsxpackages/proposal-ui/src/components/ProposalDescription/AirdropDetails/AirdropItem.tsxpackages/proposal-ui/src/components/ProposalDescription/StreamDetails/StreamItem.tsxpackages/proposal-ui/src/components/ProposalDescription/utils/feeDisplay.tspackages/proposal-ui/src/components/ProposalNavigation/ProposalNavigation.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hooks/src/useAirdropData.ts
Comment on lines
+25
to
+26
| const [titleTouched, setTitleTouched] = React.useState(false) | ||
| const [summaryTouched, setSummaryTouched] = React.useState(false) |
Contributor
There was a problem hiding this comment.
Reset the touched flags when the draft is cleared.
Because titleTouched and summaryTouched are fully local, a parent-side reset while this form stays mounted leaves them true. The next empty draft then renders required-field errors immediately, which breaks the touched-only behavior you just added. Please reset these flags from an explicit reset signal or a remount key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/create-proposal-ui/src/components/ProposalDraftForm/ProposalDraftForm.tsx`
around lines 25 - 26, The local touched flags titleTouched and summaryTouched in
ProposalDraftForm remain true when a parent clears or replaces the draft while
the form stays mounted, causing immediate validation errors; fix this by
resetting those flags whenever the draft/reset signal changes — for example, add
a useEffect in ProposalDraftForm that watches the draft prop (or a dedicated
remount/reset prop like draftId or resetCounter) and calls
setTitleTouched(false) and setSummaryTouched(false) when that prop changes so
the form returns to untouched state on a clear/replacement.
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
1 participant
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.
Summary by CodeRabbit
New Features
UX & Validation
Display