-
Notifications
You must be signed in to change notification settings - Fork 0
Fix: tags & options validation issues #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce new user interface restrictions and validation logic for poll creation and tag input. The poll form disables the "Add option" button and displays a message when the maximum of 20 options is reached. Tag input receives dynamic validation feedback, setting errors if tags are too short or too long. The time utility function for relative time is refactored to accept both start and end dates, improving formatting and removing past date handling. Calls to this function in poll components are updated accordingly. The number of polls fetched per page in the poll list is doubled from 10 to 20. The filter modal synchronizes its internal state with updated incoming props. The DateTimePicker component is refactored to restrict date selection to future dates only, disabling start date changes and start time picker. CalendarDay and TimePicker components add button type attributes and disabled styling. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PollForm
participant usePollForm
participant Validation
User->>PollForm: Click "Add option"
PollForm->>PollForm: Check number of options
alt Options < 20
PollForm->>PollForm: Enable "Add option" button
else Options >= 20
PollForm->>PollForm: Disable "Add option" button
PollForm->>User: Show "Maximum of 20 options reached"
end
User->>PollForm: Input tag
PollForm->>usePollForm: Update tagInput
usePollForm->>Validation: Validate tag length
alt Tag too short or too long
Validation->>usePollForm: Set error on "tags"
usePollForm->>PollForm: Display error message
else Tag valid
Validation->>usePollForm: Clear error on "tags"
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hooks/usePollForm.ts (1)
235-247
: Implemented real-time validation for tag input length.This is an excellent improvement that provides immediate feedback to users as they type tags. The validation correctly enforces the minimum (3 characters) and maximum (20 characters) length requirements, matching the schema validation rules.
However, there's a potential issue with clearing errors too aggressively. If there are other tag-related errors (like maximum tag count reached), they will also be cleared when the input length is valid.
Consider using a more specific error key to avoid clearing other tag errors:
-setError("tags", { +setError("tags.length", { message: `${tagInput.length}/20 Max tag character limit reached`, }); // And similarly for clearing -clearErrors("tags"); +clearErrors("tags.length");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/Poll/PollForm.tsx
(2 hunks)hooks/usePollForm.ts
(2 hunks)utils/time.ts
(1 hunks)validation/pollSchemas.ts
(1 hunks)
🔇 Additional comments (7)
validation/pollSchemas.ts (1)
34-36
: Improved code clarity by removing unused parameter.The error message callback for the tag length refinement has been simplified by removing the unused
val
parameter. This is a small but positive change that reduces unnecessary code.components/Poll/PollForm.tsx (2)
134-135
: Good UX improvement - "Add option" button now properly disabled when limit reached.The button is now disabled when the maximum of 20 options is reached, with appropriate styling to visually indicate the disabled state.
144-148
: Clear user feedback added for maximum options limit.Adding an explanatory message when the maximum number of options is reached provides clear feedback to users about why they can't add more options. This is good UX practice.
hooks/usePollForm.ts (1)
58-60
: Added form validation tools for tag validation.Correctly importing the necessary form handlers from react-hook-form to support the new tag validation functionality.
utils/time.ts (3)
9-10
: Improved time calculation with explicit isPast flag.Using a boolean flag to track whether the date is in the past makes the code more readable and intention-clear than relying on negative diff values throughout the code.
13-16
: Simplified time unit calculations.The calculation of hours and days directly from milliseconds is more efficient than the cascading approach that was likely used before.
38-41
: Updated return value with meaningful property names.The return object now uses more descriptive property names (
timeLeft
andisEnded
), making the API more intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/time.ts (1)
37-50
: Handle the edge case of 0 minutes remaining.When minutes is 0, the function will return "0m" which isn't very user-friendly. Consider implementing the suggestion from the previous review to handle zero minutes more explicitly.
You could improve the user experience by adding a special case:
} else { - timeLeft = `${minutes}m`; + timeLeft = minutes === 0 ? "0 minutes remaining" : `${minutes}m`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/Modals/FilterModal.tsx
(2 hunks)components/Poll/PollCard.tsx
(1 hunks)components/Poll/PollList.tsx
(1 hunks)components/Poll/PollResultsCard.tsx
(1 hunks)components/Poll/PollVoteCard.tsx
(1 hunks)utils/time.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
components/Poll/PollCard.tsx (1)
utils/time.ts (1)
getRelativeTimeString
(3-56)
🔇 Additional comments (10)
utils/time.ts (4)
3-6
: Improved function signature for more accurate time calculations.The function now accepts both start and end dates, allowing for more comprehensive time range validation. This is a better approach as it enables checking if the current time is outside the entire poll range.
7-24
: Enhanced error handling with meaningful defaults.The function now properly handles missing date parameters and out-of-range scenarios by immediately returning "Ended" with the appropriate flag. This will prevent downstream errors and provides clear feedback about poll status.
26-31
: Improved time calculations for more accurate results.The calculations now directly convert the millisecond difference to minutes, hours, and days instead of cascading calculations, which could introduce rounding errors.
52-55
: Consistent return structure with clear status flag.The function now returns both the formatted time string and a boolean flag indicating whether the poll has ended. This is a good pattern as it allows components to easily determine the poll status.
components/Poll/PollCard.tsx (1)
11-14
: Updated function call to match new signature.The code correctly adapts to the updated
getRelativeTimeString
function by passing both start and end dates with null handling, ensuring that the component works properly with the new time utility.components/Poll/PollResultsCard.tsx (1)
42-45
: Updated function call to match new signature.The code correctly adapts to the updated
getRelativeTimeString
function by passing both start and end dates with null handling, ensuring that the component works properly with the new time utility.components/Modals/FilterModal.tsx (2)
1-2
: Improved import organization.The imports are now better organized by grouping related React hooks together and positioning type imports before component imports.
37-39
: Added synchronization between props and internal state.This is a critical fix that ensures the internal filter state stays in sync with external filters. Without this
useEffect
, if the parent component updatesfilters
prop, the modal'stempFilters
state wouldn't reflect those changes unless manually reset.components/Poll/PollList.tsx (1)
13-13
: Increased poll pagination from 10 to 20 per page.This change doubles the number of polls loaded in each page request, which provides users with more content before needing to click "Load More".
components/Poll/PollVoteCard.tsx (1)
52-55
: Updated time calculation to use both start and end dates.The function call now correctly passes both start and end dates to the refactored
getRelativeTimeString
utility, improving accuracy of the displayed time remaining.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
hooks/usePoll.ts (1)
46-46
: Remove unnecessary empty line.This empty line after the conditional block is redundant since there's already a blank line after it. Consider removing it for cleaner code.
if (filters.isActive === "none") { return { polls: [], total: 0, }; } - const params: IUsePollParams = {
components/DateTimePicker/CalendarDay.tsx (2)
17-23
: Minor:preventDefault
is unnecessary on atype="button"
elementBecause the button is already
type="button"
(line 36) it has no default form-submission behaviour, soe.preventDefault()
is redundant. Removing it simplifies the handler.- e.preventDefault(); - if (!isDisabled) { - onClick(day, month, year); - } + if (!isDisabled) { + onClick(day, month, year); + }
35-45
: Improve accessibility by exposing disabled state to assistive techCurrently visual and functional disabling is correct, but screen-readers don’t know why the day cannot be chosen. Add
aria-disabled
so the semantic state is conveyed:- onClick={handleClick} - disabled={isDisabled} + onClick={handleClick} + disabled={isDisabled} + aria-disabled={isDisabled}components/DateTimePicker/DateTimePicker.tsx (1)
168-177
: Duplicate “is date in the past” logic – centralise & normalise time
handleDateClick
andisDateDisabled
both compare againsttoday
, but only
isDateDisabled
normalises the tested date to midnight.
For consistency (and to avoid edge-cases around time-of-day) move the normalisation to a single helper and reuse it in both places.const isPastOrToday = (d: Date) => { const todayMidnight = new Date(); todayMidnight.setHours(0,0,0,0); d.setHours(0,0,0,0); return d <= todayMidnight; };Then:
- if (clickedDate <= today) return; + if (isPastOrToday(clickedDate)) return;and
-return date <= today; +return isPastOrToday(date);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/DateTimePicker/CalendarDay.tsx
(2 hunks)components/DateTimePicker/DateTimePicker.tsx
(10 hunks)components/DateTimePicker/TimePicker.tsx
(3 hunks)components/Poll/PollList.tsx
(2 hunks)hooks/usePoll.ts
(2 hunks)hooks/usePollForm.ts
(6 hunks)types/dateTimePicker.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/DateTimePicker/TimePicker.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- components/Poll/PollList.tsx
- hooks/usePollForm.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
components/DateTimePicker/CalendarDay.tsx (2)
types/dateTimePicker.ts (1)
CalendarDayProps
(32-42)utils/index.ts (1)
cn
(8-10)
components/DateTimePicker/DateTimePicker.tsx (5)
utils/time.ts (1)
formatDate
(58-61)components/icon-components/index.tsx (1)
ClockIcon
(37-37)components/icon-components/ClockIcon.tsx (1)
ClockIcon
(3-25)components/DateTimePicker/TimePicker.tsx (1)
TimePicker
(10-112)components/ui/Button.tsx (1)
Button
(9-32)
🔇 Additional comments (5)
hooks/usePoll.ts (3)
14-14
: Good type extension for filter enhancement.Adding the
"none"
value to theisActive
filter type is a clean way to support the new filtering behavior. This allows explicit indication of when no polls should be shown, rather than using workarounds or special boolean values.
40-45
: Efficient short-circuit optimization for the "none" filter case.This early return pattern correctly implements the logic to return empty results when both active and inactive polls are filtered out. This approach prevents unnecessary network requests, which improves performance and user experience.
9-9
:❌ Incorrect review comment
Consider updating POLLS_LIMIT constant.
The AI summary mentioned that the polls fetched per page was doubled from 10 to 20, but this constant still shows 10. If the limit has indeed been changed elsewhere in the codebase, consider updating this constant for consistency.
🏁 Script executed:
#!/bin/bash # Check for any references to POLLS_LIMIT and if it's being overridden elsewhere rg "POLLS_LIMIT" --type tsLength of output: 136
No update needed for POLLS_LIMIT constant
A search of the codebase confirms
POLLS_LIMIT
is only defined here and never overridden to 20. The default of 10 is correct, so you can ignore the earlier suggestion.Likely an incorrect or invalid review comment.
types/dateTimePicker.ts (1)
40-42
: Consider makingisDisabled
optional for backwards-compatibility
isDisabled
is now mandatory for every<CalendarDay />
call-site.
If any older code (tests, storybook, admin panels, etc.) still rendersCalendarDay
you will get a compile-time error.- isDisabled: boolean; + /** Marks day as not selectable (e.g. today & past) – defaults to `false` */ + isDisabled?: boolean;The component can safely default to
false
:export const CalendarDay = ({ isDisabled = false, ...rest }: CalendarDayProps) => {/* … */}This keeps existing renders compiling while still enforcing the new rule where required.
Would you like a patch that updates the component to apply this default?
components/DateTimePicker/DateTimePicker.tsx (1)
75-84
: Minor: renamehandleTimeChange
parameter order for clarityThe callback signature
(hours, minutes)
differs from the natural “time” order(hours, minutes)
? Wait – order is correct, but adapt typed names: they are hours then minutes; Provide vantage.(This is more nitpick but not necessary; ignore)
if (clickedDate <= today) { | ||
return; | ||
} | ||
|
||
setDateRange({ | ||
...dateRange, | ||
endDate: clickedDate, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential stale-state bug when setting endDate
setDateRange
relies on the captured dateRange
value which can be stale if multiple rapid clicks occur before React processes a re-render.
Using the functional update form is safer:
- setDateRange({
- ...dateRange,
- endDate: clickedDate,
- });
+ setDateRange(prev => ({
+ ...prev,
+ endDate: clickedDate,
+ }));
📝 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.
if (clickedDate <= today) { | |
return; | |
} | |
setDateRange({ | |
...dateRange, | |
endDate: clickedDate, | |
}); | |
if (clickedDate <= today) { | |
return; | |
} | |
setDateRange(prev => ({ | |
...prev, | |
endDate: clickedDate, | |
})); |
onClick={previousMonth} | ||
className="p-2 rounded-full" | ||
className="p-2 rounded-full disabled:opacity-25" | ||
disabled={currentDate.getMonth() === today.getMonth()} | ||
aria-label="Previous month" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous month
button incorrectly disabled for future years
The button becomes disabled when the displayed month number equals today’s
month, regardless of the year (e.g. April 2026 while today is April 2025).
Add a year check so only the current month of the current year is blocked.
-disabled={currentDate.getMonth() === today.getMonth()}
+disabled={
+ currentDate.getMonth() === today.getMonth() &&
+ currentDate.getFullYear() === today.getFullYear()
+}
📝 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.
onClick={previousMonth} | |
className="p-2 rounded-full" | |
className="p-2 rounded-full disabled:opacity-25" | |
disabled={currentDate.getMonth() === today.getMonth()} | |
aria-label="Previous month" | |
onClick={previousMonth} | |
className="p-2 rounded-full disabled:opacity-25" | |
disabled={ | |
currentDate.getMonth() === today.getMonth() && | |
currentDate.getFullYear() === today.getFullYear() | |
} | |
aria-label="Previous month" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/error/page.tsx (1)
1-16
: New error page for MiniKit installation requirement.The error page provides a clear message about environment requirements and directs users to download the World App.
Consider adding these attributes to the external link for better security and user experience:
- <Link href="https://worldcoin.org/download" className="text-blue-500"> + <Link href="https://worldcoin.org/download" className="text-blue-500" target="_blank" rel="noopener noreferrer">Also, consider adding a period at the end of the last sentence for consistent punctuation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/error/page.tsx
(1 hunks)app/layout.tsx
(1 hunks)components/Login/SplashScreen.tsx
(1 hunks)components/minikit-provider.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/Login/SplashScreen.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/layout.tsx (3)
context/AuthContext.tsx (1)
AuthProvider
(38-90)components/react-query-client-provider.tsx (1)
ReactQueryClientProvider
(6-13)lib/FetchPatchProvider.ts (1)
FetchPatchProvider
(6-12)
🔇 Additional comments (3)
app/layout.tsx (1)
30-35
: Provider order changed to ensure correct context access.The order of
AuthProvider
andReactQueryClientProvider
has been swapped. This is a good change as it ensures the authentication context is available to any components making API requests through React Query.components/minikit-provider.tsx (2)
5-9
: Added router for navigation to error page.Good addition of router import and instantiation to enable error page redirection when the MiniKit validation fails.
12-15
: Added validation to ensure MiniKit is installed.This validation check is important to ensure users are accessing the app in the proper environment, with an appropriate redirect to the error page if requirements aren't met.
Summary by CodeRabbit
New Features
Improvements
Style