Conversation
…Password required in login schema
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This pull request addresses build issues by refactoring form components and schemas. The changes primarily involve migrating from Formik to react-hook-form, improving type safety, and fixing schema definitions across various form components.
Key Changes:
- Migrated form input components (TextInput, TextareaInput, SelectInput, CheckboxInput, InputWithDropDown) from Formik's Field-based approach to native HTML elements with forwardRef support for react-hook-form compatibility
- Enhanced type safety in TodoForm.tsx and MeetingForm.tsx by adding explicit type assertions and definitions
- Removed unused
usersLoadingprop from TodoModel component - Updated Zod schemas to improve validation logic
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/feature/todo/components/TodoModel.tsx | Removed unused usersLoading prop from TodoForm component |
| src/feature/todo/components/TodoForm.tsx | Added type safety with Status and Priority types, added type assertions for dropdown onChange handlers, and reformatted code |
| src/feature/meetings/components/MeetingForm.tsx | Updated Zod schema by removing .default() and .or(z.literal("")) chains, simplified status type conversion logic |
| src/components/ui/TextareaInput.tsx | Migrated from Formik Field component to native textarea with forwardRef for react-hook-form compatibility |
| src/components/ui/TextInput.tsx | Migrated from Formik Field component to native input with forwardRef for react-hook-form compatibility |
| src/components/ui/SelectInput.tsx | Migrated from Formik Field component to native select with forwardRef for react-hook-form compatibility |
| src/components/ui/InputWithDropDown.tsx | Migrated from Formik Field components to native input/select elements with ref support |
| src/components/ui/CheckboxInput.tsx | Migrated from Formik Field component to native checkbox input with forwardRef for react-hook-form compatibility |
| src/components/AuthModel/page.tsx | Updated Zod schema definitions for email validation and login schema fields |
Comments suppressed due to low confidence (3)
src/feature/todo/components/TodoModel.tsx:9
- Unused import toast.
import toast from "react-hot-toast"
src/feature/todo/components/TodoModel.tsx:12
- Unused import useCompaniesStore.
import { useCompaniesStore } from "@/feature/companies/stores/useCompaniesStore"
src/feature/todo/components/TodoModel.tsx:25
- Unused variable usersLoading.
const { users, loading: usersLoading, fetchUsers } = useUserStore();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const baseSchema = z.object({ | ||
| email: z.string().email("Invalid email"), | ||
| email: z.email("Invalid email"), |
There was a problem hiding this comment.
The method z.email() does not exist in Zod. It should be z.string().email("Invalid email") to validate email format.
| email: z.email("Invalid email"), | |
| email: z.string().email("Invalid email"), |
| name: z.string(), | ||
| confirmPassword: z.string(), |
There was a problem hiding this comment.
In the loginSchema, name and confirmPassword fields should be .optional() since they are only required for signup, not login. Without .optional(), these fields will be required for login validation, which is incorrect.
| name: z.string(), | |
| confirmPassword: z.string(), | |
| name: z.string().optional(), | |
| confirmPassword: z.string().optional(), |
| step="0.01" | ||
| min="0" | ||
| ref={inputRef} | ||
| className="flex-1 text-sm rounded-l-md px-3 py-2 outline-none [&::-webkit-inner-spin-button]:appearance-none [&:-webkit-outer-spin-button]:appearance-none [appearance:textfield] w-1/4" |
There was a problem hiding this comment.
Invalid CSS pseudo-class selector. The correct syntax is [&::-webkit-outer-spin-button] (with double colons), not [&:-webkit-outer-spin-button] (single colon). This will cause the style to not be applied correctly.
| className="flex-1 text-sm rounded-l-md px-3 py-2 outline-none [&::-webkit-inner-spin-button]:appearance-none [&:-webkit-outer-spin-button]:appearance-none [appearance:textfield] w-1/4" | |
| className="flex-1 text-sm rounded-l-md px-3 py-2 outline-none [&::-webkit-inner-spin-button]:appearance-none [&::-webkit-outer-spin-button]:appearance-none [appearance:textfield] w-1/4" |
| assignedTo: z.string().optional(), | ||
| linkedTo: z.string().optional(), | ||
| participants: z.array(z.string().trim().min(1, "Participant name cannot be empty").max(100)), | ||
| status: z.enum(["scheduled", "confirmed", "cancelled"], { error: "Status is required" }), |
There was a problem hiding this comment.
The z.enum() configuration object should use required_error or errorMap, not error. The property name error is not valid for Zod's enum options and will not display the custom error message.
| status: z.enum(["scheduled", "confirmed", "cancelled"], { error: "Status is required" }), | |
| status: z.enum(["scheduled", "confirmed", "cancelled"], { required_error: "Status is required" }), |
| tags: z.array(z.string().trim().min(1, "Tag cannot be empty").max(30)).default([]), | ||
| assignedTo: z.string().optional(), | ||
| linkedTo: z.string().optional(), | ||
| participants: z.array(z.string().trim().min(1, "Participant name cannot be empty").max(100)), |
There was a problem hiding this comment.
The participants field should have a .default([]) to handle cases where participants are not provided. Without it, the field will be undefined when not provided, which can cause runtime errors when the code expects an array (e.g., in line 242 where vals.participants.map() is called).
| participants: z.array(z.string().trim().min(1, "Participant name cannot be empty").max(100)), | |
| participants: z.array(z.string().trim().min(1, "Participant name cannot be empty").max(100)).default([]), |
| linkedTo: z.string().optional(), | ||
| participants: z.array(z.string().trim().min(1, "Participant name cannot be empty").max(100)), | ||
| status: z.enum(["scheduled", "confirmed", "cancelled"], { error: "Status is required" }), | ||
| tags: z.array(z.string()).optional(), |
There was a problem hiding this comment.
The tags field should have a .default([]) to ensure it's always an array. Without this, when tags are not provided, the field will be undefined, which can cause issues when the code expects an array.
| tags: z.array(z.string()).optional(), | |
| tags: z.array(z.string()).optional().default([]), |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
…r improved flexibility' (#15) from implement/semantic_release into main Reviewed-on: https://git.stralya.com/Klickbee/klickbee-crm/pulls/15
No description provided.