Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements QA feedback for the goal page, featuring significant architectural refactoring and UI improvements.
Key Changes:
- Refactored goal creation to use Zod validation with form resolvers, replacing manual validation
- Enhanced ToolTip component to support multi-line content and independent tail positioning
- Consolidated API layer by renaming hooks to queries and adding proper DTOs
Reviewed changes
Copilot reviewed 55 out of 57 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Added dependencies for form validation (@hookform/resolvers, zod v3), date handling (date-fns, date-holidays) |
| src/shared/components/display/ToolTip.tsx | Enhanced tooltip to accept ReactNode content, added tailPosition prop and displayName |
| src/model/goal/dto.ts | New file defining Zod schema for goal creation request validation |
| src/model/goal/queries.ts | Renamed from hooks.ts, updated createGoal mutation to use new DTO types |
| src/model/goal/api.ts | Added postGoal method using CreateGoalRequestType |
| src/feature/goal/createGoalFormElement/dto.ts | New file with form-specific validation including duration range checks |
| src/feature/goal/createGoalFormElement/component.tsx | Integrated Zod resolver, removed Context pattern, added DurationErrorMessage component |
| src/feature/goal/confimGoal/api.ts | Updated type references (incomplete migration with type mismatch) |
| src/composite/goal/planetSelectSection/index.tsx | Renamed from PlanetSelector, added GoalProgressSheet, hardcoded year to 2025 |
| src/composite/goal/planetSelectSection/components/PlanetItem.tsx | Updated tooltip to use multi-line text, adjusted sizing and positioning |
| src/app/(home)/goal/page.tsx | Updated import paths, extracted GoalPageOverlay component |
| src/app/(home)/goal/create-onboard/page.tsx | Simplified goal creation payload to match new DTO structure |
| src/composite/advice/index.tsx | Updated imports from hooks to queries |
| Multiple deleted files | Removed unused features: weeklyGoalBoard, planBoard, retrospect components, mentor cards |
Comments suppressed due to low confidence (3)
src/composite/goal/planetSelectSection/index.tsx:25
- The year is hardcoded to 2025 in the GoalProvider. This will cause issues when the calendar year changes to 2026 or beyond, as it will only show goals from 2025. Consider using
new Date().getFullYear()to dynamically get the current year instead of hardcoding it.
src/shared/components/display/ToolTip.tsx:61 - The tail border color uses
border-t-label-normalandborder-b-label-normal, but the tooltip background is white (bg-white). This creates a visual mismatch where the tail color won't match the tooltip body. The tail border colors should be changed to useborder-t-whiteandborder-b-whiteto match the white background of the tooltip.
src/composite/goal/planetSelectSection/components/PlanetItem.tsx:106 - The tooltip has
className="inset-0"which positions it to cover the entire button area (all edges set to 0). Combined withposition="top-center"which addsbottom-fullpositioning, this creates conflicting positioning rules. Theinset-0will override the positioning fromgetPositionClasses, causing the tooltip to appear incorrectly. Either remove theinset-0class or use a more appropriate positioning class that works with the top-center position.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| duration: z.object({ | ||
| startDate: z.string().min(1, '시작일을 선택해주세요'), | ||
| endDate: z.string().min(1, '종료일을 선택해주세요'), | ||
| }), |
There was a problem hiding this comment.
The date validation only checks that the strings are non-empty using min(1), but doesn't validate that they are in the correct date format (YYYY-MM-DD) or that they are valid dates. Consider using z.string().datetime() or a custom refinement with z.string().refine() to ensure the date strings are properly formatted and valid before they reach the form-level validation.
| return ( | ||
| <div className={`absolute z-10 ${getPositionClasses(position)} ${className}`}> | ||
| <div className="relative py-2 px-3 bg-white rounded-xl caption-1-bold whitespace-nowrap text-black shadow-sm"> | ||
| <div className="relative py-2 px-3 bg-white rounded-xl label-2-medium whitespace-nowrap text-black shadow-sm"> |
There was a problem hiding this comment.
The text style class has been changed from caption-1-bold to label-2-medium, which changes both the font size and weight. This is a breaking change that will affect all existing tooltips in the application. Ensure this change is intentional and aligns with the design system, as it will make tooltips appear differently across the entire application.
src/feature/goal/confimGoal/api.ts
Outdated
| export async function postCreateGoal(formData: GoalFormData) { | ||
| const { duration, durationDate, ...rest } = formData; | ||
| const dto: CreateGoalRequestDto = { | ||
| const dto: CreateGoalRequestType = { | ||
| ...rest, | ||
| duration: durationDate, | ||
| }; |
There was a problem hiding this comment.
The function postCreateGoal still uses the old GoalFormData type parameter, but attempts to destructure and transform it into CreateGoalRequestType. This creates a type mismatch since GoalFormData has fields like category, toBe, and plans that aren't present in CreateGoalRequestType. The destructuring { duration, durationDate, ...rest } will include these extra fields in rest, which will fail type checking when assigning to CreateGoalRequestType.
This function appears to be unused (only called from hook.ts which is not exported anywhere), so it should either be removed or updated to match the new type system using CreateGoalRequestType as the parameter type.
| return ( | ||
| <div className={`absolute z-10 ${getPositionClasses(position)} ${className}`}> | ||
| <div className="relative py-2 px-3 bg-white rounded-xl caption-1-bold whitespace-nowrap text-black shadow-sm"> | ||
| <div className="relative py-2 px-3 bg-white rounded-xl label-2-medium whitespace-nowrap text-black shadow-sm"> |
There was a problem hiding this comment.
The tooltip text type has been changed from string to string | React.ReactNode, but the whitespace-nowrap class on line 91 is still applied. This will prevent multi-line content (like the <br /> tag used in PlanetItem.tsx) from wrapping properly. The nowrap class should be removed or made conditional based on the text type to support the new multi-line tooltip feature.
| const DurationErrorMessage = () => { | ||
| const { | ||
| formState: { errors }, | ||
| } = useFormContext<CreateGoalFormType>(); | ||
|
|
||
| if (!errors || !errors.duration) return null; | ||
|
|
||
| return ( | ||
| <p className="caption-1-medium text-text-danger mt-2 flex items-center gap-x-2"> | ||
| <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path | ||
| d="M8.00016 14.6668C4.31816 14.6668 1.3335 11.6822 1.3335 8.00016C1.3335 4.31816 4.31816 1.3335 8.00016 1.3335C11.6822 1.3335 14.6668 4.31816 14.6668 8.00016C14.6668 11.6822 11.6822 14.6668 8.00016 14.6668ZM8.00016 13.3335C9.41465 13.3335 10.7712 12.7716 11.7714 11.7714C12.7716 10.7712 13.3335 9.41465 13.3335 8.00016C13.3335 6.58567 12.7716 5.22912 11.7714 4.22893C10.7712 3.22873 9.41465 2.66683 8.00016 2.66683C6.58567 2.66683 5.22912 3.22873 4.22893 4.22893C3.22873 5.22912 2.66683 6.58567 2.66683 8.00016C2.66683 9.41465 3.22873 10.7712 4.22893 11.7714C5.22912 12.7716 6.58567 13.3335 8.00016 13.3335ZM8.00016 4.66683C8.17697 4.66683 8.34654 4.73707 8.47157 4.86209C8.59659 4.98712 8.66683 5.15669 8.66683 5.3335V8.66683C8.66683 8.84364 8.59659 9.01321 8.47157 9.13823C8.34654 9.26326 8.17697 9.3335 8.00016 9.3335C7.82335 9.3335 7.65378 9.26326 7.52876 9.13823C7.40373 9.01321 7.3335 8.84364 7.3335 8.66683V5.3335C7.3335 5.15669 7.40373 4.98712 7.52876 4.86209C7.65378 4.73707 7.82335 4.66683 8.00016 4.66683ZM8.00016 11.3335C7.82335 11.3335 7.65378 11.2633 7.52876 11.1382C7.40373 11.0132 7.3335 10.8436 7.3335 10.6668C7.3335 10.49 7.40373 10.3204 7.52876 10.1954C7.65378 10.0704 7.82335 10.0002 8.00016 10.0002C8.17697 10.0002 8.34654 10.0704 8.47157 10.1954C8.59659 10.3204 8.66683 10.49 8.66683 10.6668C8.66683 10.8436 8.59659 11.0132 8.47157 11.1382C8.34654 11.2633 8.17697 11.3335 8.00016 11.3335Z" | ||
| fill="#FF6363" | ||
| /> | ||
| </svg> | ||
| {errors.duration.message} | ||
| </p> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The error message component checks for errors.duration but the zod refinement sets the error path to ['duration'] which creates a root-level error on the duration object. However, the individual field errors (duration.startDate and duration.endDate) are set on nested paths. This means if a user hasn't filled in the startDate or endDate, those errors won't be displayed since they're at errors.duration.startDate and errors.duration.endDate, not errors.duration. The component should check for both the root duration error and the nested field errors to provide comprehensive validation feedback.
| export const createGoalFormSchema = createGoalRequestSchema.refine( | ||
| data => { | ||
| const start = new Date(data.duration.startDate); | ||
| const end = new Date(data.duration.endDate); | ||
|
|
||
| const diffTime = end.getTime() - start.getTime(); | ||
| const diffDays = diffTime / (1000 * 60 * 60 * 24); | ||
|
|
||
| // 최소 7일(1주일), 최대 365일 (1년) | ||
| return diffDays >= 7 && diffDays <= 365; | ||
| }, | ||
| { | ||
| message: '목표 기간은 최소 1주, 최대 1년까지 설정 가능합니다.', | ||
| path: ['duration'], | ||
| } |
There was a problem hiding this comment.
The date range validation calculates the difference in days and checks if it's between 7 and 365 days. However, this doesn't account for the edge case where startDate is after endDate, which would result in a negative diffDays value that would fail the validation. While this provides the correct validation result, the error message "목표 기간은 최소 1주, 최대 1년까지 설정 가능합니다." is misleading when the issue is that the end date is before the start date. Consider adding a separate validation check for this case with a more specific error message.
src/feature/goal/confimGoal/api.ts
Outdated
| @@ -1,22 +1,8 @@ | |||
| import { CreateGoalRequestType } from '@/model/goal/dto'; | |||
There was a problem hiding this comment.
The directory is named "confimGoal" but it should be "confirmGoal" (missing an 'r'). This is a spelling error in the directory name that affects the import paths throughout the codebase.
No description provided.