Conversation
- 입력값 : 현재 인터뷰 일정(날짜/시간) , 날짜, 선택된 시간 - 업데이트된 인터뷰 일정(날짜/시간)
- 초기 상태, Action
- updateSchedule util 함수 적용
- hover 반응, border, color
- 기존 : 값이 없어도 [] 전송 - 현재 : 값이 존재하지 않으면 아예 전송X
- 마우스 상태에 따른 state를 일괄적으로 관리
- Id -> clubID
- object -> state length 매개 변수function
- 범용성 & 특정 도메인에 종속적 여부로 판단
- 특정 도메인에 종속X 판단
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces reducer-driven drag-selection for interview scheduling: new drag types, utilities, and initial state; replaces legacy form updater with a hook that persists merged time intervals to react-hook-form; updates component usage and styles; adjusts DTO to omit empty interviewDateAnswer; renames useApplicationForm parameter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Selector as InterviewScheduleSelector
participant DragHook as useDragSelection (reducer)
participant FormHook as useInterviewScheduleUpdater
participant Domain as updateSchedule
participant RHF as react-hook-form
User->>Selector: mousedown / mousemove / mouseup
Selector->>DragHook: handleMouseDown/Move/Up(event index)
DragHook->>DragHook: reducer updates DragState\n(startIndex, hover, mode, isSelectedStates)
Note over DragHook: on mouseup, final isSelectedStates produced
DragHook->>FormHook: updateInterviewSchedule(isSelectedStates)
FormHook->>Domain: updateSchedule(currentSchedules, date, mergedTimes)
Domain-->>FormHook: newSchedules
FormHook->>RHF: setValue('selectedInterviewSchedule', newSchedules)
RHF-->>Selector: form state updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
Summary of ChangesHello @aaaaaattt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 지원 폼 제출 기능의 코드 구조를 리팩토링하고 사용자 경험을 개선하는 데 중점을 둡니다. 특히 인터뷰 일정 선택 로직을 도메인과 유틸리티 계층으로 분리하고 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
전반적으로 매우 훌륭한 리팩터링입니다. 지원서 제출 관련 로직의 관심사를 성공적으로 분리하셨습니다. 복잡한 드래그 상태 관리를 useReducer로 이전하여 응집도를 높이고, 도메인 로직과 유틸리티 함수를 분리하여 코드의 구조를 명확하게 만든 점이 인상적입니다. 특히 도메인 로직 분리 기준에 대해 고민하신 부분이 코드에 잘 드러나며, 유지보수성과 가독성이 크게 향상되었습니다. 몇 가지 추가적인 개선 제안을 포함했으니 확인 부탁드립니다.
| onMouseUp={handleMouseUp} | ||
| > | ||
| <Text>{e[0] + '~' + e[1]}</Text> | ||
| <Text size='xs'>{e[0] + '~' + e[1]}</Text> |
There was a problem hiding this comment.
문자열을 연결할 때 + 연산자 대신 템플릿 리터럴(`${...}`)을 사용하면 코드가 더 깔끔하고 가독성이 좋아집니다.
| <Text size='xs'>{e[0] + '~' + e[1]}</Text> | |
| <Text size='xs'>{`${e[0]}~${e[1]}`}</Text> |
| if (idxDiffSign < 0) { | ||
| newStartIndex = curtIdx + 1; | ||
| } else if (idxDiffSign > 0) { | ||
| newStartIndex = curtIdx - 1; | ||
| } else { | ||
| newStartIndex = curtIdx; | ||
| } |
There was a problem hiding this comment.
newStartIndex를 계산하는 if/else if/else 블록은 newStartIndex = curtIdx - idxDiffSign; 한 줄로 간단하게 표현할 수 있습니다. idxDiffSign이 1, -1, 0인 모든 경우를 처리할 수 있어 코드가 더 간결해집니다.
newStartIndex = curtIdx - idxDiffSign;
src/pages/user/Apply/type/apply.ts
Outdated
| | { type: 'mouseDown'; index: number; isSelectionMode: boolean } | ||
| | { type: 'mouseMove'; index: number; indexDiffSign: number } | ||
| | { type: 'mouseUp' } | ||
| | { type: 'setInitialDragDirection'; indexDiffSign: number }; |
There was a problem hiding this comment.
DragAction 타입에 정의된 setInitialDragDirection 액션 타입이 dragReducer에서 사용되지 않고 있습니다. 불필요한 코드는 혼란을 줄 수 있으므로 제거하는 것이 좋습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/pages/user/Apply/utils/math.ts (1)
9-15: Helpers are correct but consider simplifying.The implementations are correct and compose well with the existing
getSignfunction. However,getDiffis a thin wrapper around subtraction.Consider simplifying
getIndexDiffSignto remove the intermediategetDiff:-export function getDiff(A: number, B: number): number { - return A - B; -} - export function getIndexDiffSign(currentSelectedIndex: number, lastHoveredIndex: number): Sign { - return getSign(getDiff(currentSelectedIndex, lastHoveredIndex)); + return getSign(currentSelectedIndex - lastHoveredIndex); }This reduces indirection while maintaining clarity. Keep the change if you prefer the explicit naming of
getDifffor semantic clarity.src/pages/user/Apply/hook/useApplicationForm.ts (1)
5-5: Rename fetchApplicationForm parameter toclubId
In src/pages/user/Apply/api/apply.ts (line 8), update the function signature from(Id: number)to(clubId: number)and adjust all internal references to match.src/pages/user/Apply/domain/schedule.ts (2)
6-15: Param naming + redundant coalescing
- Parameter name selectedTime (singular) holds an array; prefer selectedTimes for clarity.
- selectedTime ?? [] is redundant because the param is typed as string[].
Consider this small cleanup:
export const updateSchedule = ( currentInterviewSchedule: PostInterviewSchedule[], date: string, - selectedTime: string[], + selectedTimes: string[], ): PostInterviewSchedule[] => { @@ const selectedInterviewSchedule: PostInterviewSchedule = { - date: date, - selectedTimes: selectedTime ?? [], + date, + selectedTimes, };
18-25: Option: drop empty selections instead of storing empty entriesIf mergedTimes is empty, you may want to remove that date from the schedule to reduce payload and simplify DTO handling. Verify product intent.
if (sameDateIndex !== -1) { - updatedSchedule = [...currentInterviewSchedule]; - updatedSchedule[sameDateIndex] = selectedInterviewSchedule; + if (selectedInterviewSchedule.selectedTimes.length === 0) { + updatedSchedule = currentInterviewSchedule.filter((s) => s.date !== date); + } else { + updatedSchedule = [...currentInterviewSchedule]; + updatedSchedule[sameDateIndex] = selectedInterviewSchedule; + } } else { - updatedSchedule = [...currentInterviewSchedule, selectedInterviewSchedule]; + updatedSchedule = + selectedInterviewSchedule.selectedTimes.length === 0 + ? currentInterviewSchedule + : [...currentInterviewSchedule, selectedInterviewSchedule]; }src/pages/user/Apply/hook/useFormDataUpdate.ts (2)
15-16: Mark form field dirty when updating schedulesEnsure formState reflects the update.
- setValue('selectedInterviewSchedule', updatedSchedules); + setValue('selectedInterviewSchedule', updatedSchedules, { shouldDirty: true });
12-16: Optional: filter out empty dates before persistingTo avoid storing empty selections:
- const updatedSchedules = updateSchedule(currentSchedules, date, mergedTimes); - setValue('selectedInterviewSchedule', updatedSchedules, { shouldDirty: true }); + const updatedSchedules = updateSchedule(currentSchedules, date, mergedTimes); + const nonEmpty = updatedSchedules.filter((s) => s.selectedTimes.length > 0); + setValue('selectedInterviewSchedule', nonEmpty, { shouldDirty: true });src/pages/user/Apply/domain/drag.ts (1)
9-13: Narrow idxDiffSign type and rename curtIdx for clarityConstrain to -1 | 0 | 1 and use a clearer name.
-export const updateDragState = ( - state: DragState, - curtIdx: number, - idxDiffSign: number, -): UpdatedDragState => { +export const updateDragState = ( + state: DragState, + currIdx: number, + idxDiffSign: -1 | 0 | 1, +): UpdatedDragState => {- if (idxDiffSign < 0) { - newStartIndex = curtIdx + 1; - } else if (idxDiffSign > 0) { - newStartIndex = curtIdx - 1; - } else { - newStartIndex = curtIdx; + if (idxDiffSign < 0) { + newStartIndex = currIdx + 1; + } else if (idxDiffSign > 0) { + newStartIndex = currIdx - 1; + } else { + newStartIndex = currIdx; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/pages/user/Apply/api/apply.ts(2 hunks)src/pages/user/Apply/components/ApplicationForm/InterviewScheduleSelector.tsx(2 hunks)src/pages/user/Apply/components/ApplicationForm/index.styled.ts(2 hunks)src/pages/user/Apply/constant/initialDragState.ts(1 hunks)src/pages/user/Apply/domain/drag.ts(1 hunks)src/pages/user/Apply/domain/schedule.ts(1 hunks)src/pages/user/Apply/hook/useApplicationForm.ts(1 hunks)src/pages/user/Apply/hook/useDragSelection.ts(1 hunks)src/pages/user/Apply/hook/useFormDataUpdate.ts(1 hunks)src/pages/user/Apply/hook/useUpdateFormData.ts(0 hunks)src/pages/user/Apply/type/apply.ts(1 hunks)src/pages/user/Apply/utils/drag.ts(1 hunks)src/pages/user/Apply/utils/math.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/pages/user/Apply/hook/useUpdateFormData.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/pages/user/Apply/constant/initialDragState.ts (1)
src/pages/user/Apply/type/apply.ts (1)
DragState(71-80)
src/pages/user/Apply/hook/useApplicationForm.ts (2)
src/pages/user/Apply/components/ApplicationForm/index.tsx (1)
ApplicationForm(18-208)src/pages/user/Apply/api/apply.ts (1)
fetchApplicationForm(8-14)
src/pages/user/Apply/domain/schedule.ts (1)
src/pages/user/Apply/type/apply.ts (1)
PostInterviewSchedule(56-59)
src/pages/user/Apply/hook/useFormDataUpdate.ts (3)
src/pages/user/Apply/type/apply.ts (2)
FormInputs(61-69)PostInterviewSchedule(56-59)src/pages/user/Apply/utils/time.ts (2)
convertSelectionToTimeInterval(58-74)mergeContinuousTimeInterval(76-106)src/pages/user/Apply/domain/schedule.ts (1)
updateSchedule(3-26)
src/pages/user/Apply/components/ApplicationForm/InterviewScheduleSelector.tsx (2)
src/pages/user/Apply/hook/useDragSelection.ts (1)
useDragSelection(71-118)src/shared/components/Text/index.tsx (1)
Text(12-23)
src/pages/user/Apply/hook/useDragSelection.ts (6)
src/pages/user/Apply/type/apply.ts (2)
DragState(71-80)DragAction(82-86)src/pages/user/Apply/domain/drag.ts (1)
updateDragState(9-33)src/pages/user/Apply/utils/drag.ts (1)
updateSelectedState(1-13)src/pages/user/Apply/hook/useFormDataUpdate.ts (1)
useInterviewScheduleUpdater(6-19)src/pages/user/Apply/constant/initialDragState.ts (1)
generateInitialDragState(3-14)src/pages/user/Apply/utils/math.ts (1)
getIndexDiffSign(13-15)
src/pages/user/Apply/components/ApplicationForm/index.styled.ts (1)
src/styles/theme.ts (1)
theme(3-10)
src/pages/user/Apply/domain/drag.ts (1)
src/pages/user/Apply/type/apply.ts (1)
DragState(71-80)
🪛 GitHub Actions: Dongarium FE CI/CD
src/pages/user/Apply/hook/useDragSelection.ts
[error] 2-2: ESLint: '../utils/math import should occur after import of ../utils/drag' import/order
🪛 GitHub Check: lint
src/pages/user/Apply/hook/useFormDataUpdate.ts
[failure] 4-4:
../domain/schedule import should occur before type import of ../type/apply
[failure] 3-3:
../utils/time import should occur before type import of ../type/apply
src/pages/user/Apply/hook/useDragSelection.ts
[failure] 4-4:
../type/apply type import should occur after import of ../utils/drag
[failure] 2-2:
../utils/math import should occur after import of ../utils/drag
🔇 Additional comments (10)
src/pages/user/Apply/components/ApplicationForm/index.styled.ts (2)
99-107: LGTM! Enhanced drag selection UI feedback.The styling improvements appropriately support the drag-and-select interface:
- Proper border definition and radius for visual clarity
- Smooth transitions for selection state changes
- Interactive hover feedback that respects selection state
- Color contrast improvement when selected
120-120: Font size reduction aligns with UI scale.The change from
basetosmappropriately scales the date text to match the time slot UI proportions.src/pages/user/Apply/type/apply.ts (1)
71-86: LGTM! Well-structured drag state types.The type definitions appropriately support reducer-driven drag handling:
DragStatecaptures all necessary drag interaction stateDragActionuses discriminated union pattern for type-safe action handling- Properties are well-named and comprehensive
src/pages/user/Apply/constant/initialDragState.ts (1)
3-14: LGTM! Clean initial state generator.The function correctly initializes all drag state properties with appropriate defaults:
- Index values at -1 indicate no selection
- Boolean flags properly initialized to false
- Array length matches the provided statesLength
src/pages/user/Apply/utils/drag.ts (1)
1-13: LGTM! Clean range selection utility.The implementation correctly handles bidirectional selection:
Math.min/Math.maxnormalize the range direction- Immutable update via
map- Inclusive range logic is appropriate for UI selection
src/pages/user/Apply/components/ApplicationForm/InterviewScheduleSelector.tsx (3)
10-13: LGTM! Clean integration with refactored drag hook.The component correctly integrates with the new
useDragSelectionhook, receiving both state and handlers for the drag-and-select interaction.
23-23: Selection state properly sourced from hook.The change from
selectedTime[idx]tostates.isSelectedStates[idx]correctly uses the state from the refactored drag selection hook.
28-28: Text size aligns with styling updates.The
size='xs'prop matches the font size reduction in the styled component (Line 120 of index.styled.ts).src/pages/user/Apply/api/apply.ts (1)
38-42: Good bug fix: avoid sending empty interview dates.The conditional logic correctly prevents appending an empty
interviewDateAnswerto the form data, avoiding unnecessary empty fields in the API request.src/pages/user/Apply/hook/useDragSelection.ts (1)
109-110: Order of operations on mouseUp is fine; state used is stableCalling updateInterviewSchedule after dispatch('mouseUp') is safe since mouseUp doesn’t modify isSelectedStates. LGTM.
| import { useReducer } from 'react'; | ||
| import { getIndexDiffSign } from '../utils/math'; | ||
| import { useInterviewScheduleUpdater } from './useFormDataUpdate'; | ||
| import type { DragAction, DragState } from '../type/apply'; | ||
|
|
||
| export function useDragSelection( | ||
| updateScheduleData: (data: string, mergedInterviewTime: string[]) => void, | ||
| date: string, | ||
| timeIntervalArray: [string, string][], | ||
| ) { | ||
| const [prevDiffSign, setPrevDiffSign] = useState<Sign>(0); | ||
| const startIndex = useRef<string | undefined>(''); | ||
| const lastHoveredIndex = useRef<string | null>(null); | ||
| const mode = useRef<boolean>(false); | ||
| const [isDragging, setIsDragging] = useState<boolean>(false); | ||
| const [isMouseDown, setIsMouseDown] = useState<boolean>(false); | ||
| const selectedIndex = useRef<string | undefined>(''); | ||
| const [selectedTime, setSelectedTime] = useState<boolean[]>(() => | ||
| new Array(timeIntervalArray.length).fill(false), | ||
| ); | ||
|
|
||
| function handleIndexChange(newIndex: number) { | ||
| const diff = newIndex - Number(lastHoveredIndex.current); | ||
| const currentSign = getSign(diff); | ||
| import { generateInitialDragState } from '../constant/initialDragState'; | ||
| import { updateDragState } from '../domain/drag'; | ||
| import { updateSelectedState } from '../utils/drag'; | ||
|
|
There was a problem hiding this comment.
Fix import order (ESLint import/order) and add MouseEvent type
Reorder imports to place ../utils/drag before ../utils/math and keep type-only imports after value imports. Also import MouseEvent type to avoid relying on global React namespace.
-import { useReducer } from 'react';
-import { getIndexDiffSign } from '../utils/math';
-import { useInterviewScheduleUpdater } from './useFormDataUpdate';
-import type { DragAction, DragState } from '../type/apply';
-
-import { generateInitialDragState } from '../constant/initialDragState';
-import { updateDragState } from '../domain/drag';
-import { updateSelectedState } from '../utils/drag';
+import { useReducer } from 'react';
+import type { MouseEvent } from 'react';
+import { updateSelectedState } from '../utils/drag';
+import { getIndexDiffSign } from '../utils/math';
+import { useInterviewScheduleUpdater } from './useFormDataUpdate';
+import { generateInitialDragState } from '../constant/initialDragState';
+import { updateDragState } from '../domain/drag';
+import type { DragAction, DragState } from '../type/apply';📝 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.
| import { useReducer } from 'react'; | |
| import { getIndexDiffSign } from '../utils/math'; | |
| import { useInterviewScheduleUpdater } from './useFormDataUpdate'; | |
| import type { DragAction, DragState } from '../type/apply'; | |
| export function useDragSelection( | |
| updateScheduleData: (data: string, mergedInterviewTime: string[]) => void, | |
| date: string, | |
| timeIntervalArray: [string, string][], | |
| ) { | |
| const [prevDiffSign, setPrevDiffSign] = useState<Sign>(0); | |
| const startIndex = useRef<string | undefined>(''); | |
| const lastHoveredIndex = useRef<string | null>(null); | |
| const mode = useRef<boolean>(false); | |
| const [isDragging, setIsDragging] = useState<boolean>(false); | |
| const [isMouseDown, setIsMouseDown] = useState<boolean>(false); | |
| const selectedIndex = useRef<string | undefined>(''); | |
| const [selectedTime, setSelectedTime] = useState<boolean[]>(() => | |
| new Array(timeIntervalArray.length).fill(false), | |
| ); | |
| function handleIndexChange(newIndex: number) { | |
| const diff = newIndex - Number(lastHoveredIndex.current); | |
| const currentSign = getSign(diff); | |
| import { generateInitialDragState } from '../constant/initialDragState'; | |
| import { updateDragState } from '../domain/drag'; | |
| import { updateSelectedState } from '../utils/drag'; | |
| // src/pages/user/Apply/hook/useDragSelection.ts | |
| import { useReducer } from 'react'; | |
| import type { MouseEvent } from 'react'; | |
| import { updateSelectedState } from '../utils/drag'; | |
| import { getIndexDiffSign } from '../utils/math'; | |
| import { useInterviewScheduleUpdater } from './useFormDataUpdate'; | |
| import { generateInitialDragState } from '../constant/initialDragState'; | |
| import { updateDragState } from '../domain/drag'; | |
| import type { DragAction, DragState } from '../type/apply'; | |
| // …rest of the file… |
🧰 Tools
🪛 GitHub Actions: Dongarium FE CI/CD
[error] 2-2: ESLint: '../utils/math import should occur after import of ../utils/drag' import/order
🪛 GitHub Check: lint
[failure] 4-4:
../type/apply type import should occur after import of ../utils/drag
[failure] 2-2:
../utils/math import should occur after import of ../utils/drag
🤖 Prompt for AI Agents
In src/pages/user/Apply/hook/useDragSelection.ts around lines 1 to 9, the
imports are misordered and the MouseEvent type is missing; reorder so value
imports come first, move "../utils/drag" before "../utils/math", place type-only
imports (like DragAction, DragState) after the value imports, and add an
explicit type import for MouseEvent from 'react' (import type { MouseEvent }
from 'react') to avoid relying on the global React namespace.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/pages/user/Apply/domain/drag.ts (1)
21-27: Simplify start-index calculationEquivalent, shorter, and less error-prone.
- if (idxDiffSign < 0) { - newStartIndex = currentIdx + 1; - } else if (idxDiffSign > 0) { - newStartIndex = currentIdx - 1; - } else { - newStartIndex = currentIdx; - } + newStartIndex = currentIdx - idxDiffSign;src/pages/user/Apply/hook/useDragSelection.ts (4)
1-9: Import MouseEvent type and useEffect; keep import order sanePrevents reliance on global React namespace and enables reset effect.
-import { useReducer } from 'react'; +import { useEffect, useReducer } from 'react'; +import type { MouseEvent } from 'react';
1-9: Import order now looks correct; ensure lint passesLooks aligned with prior lint feedback; adding the type-only import may require keeping it after value imports as shown.
88-101: De-duplicate index lookups and guard invalid state on mouseMovePrevents unnecessary work and invalid dispatch.
- const handleMouseMove = (e: React.MouseEvent<HTMLSpanElement>) => { - if ( - !e.currentTarget.dataset.index || - getSelectedIndex(e) === states.lastHoveredIndex || - !states.isMouseDown - ) - return; - dispatch({ - type: 'mouseMove', - index: getSelectedIndex(e), - indexDiffSign: getIndexDiffSign(getSelectedIndex(e), states.lastHoveredIndex), - }); + const handleMouseMove = (e: MouseEvent<HTMLSpanElement>) => { + const index = getSelectedIndex(e); + if (!e.currentTarget.dataset.index || index === states.lastHoveredIndex || !states.isMouseDown) + return; + dispatch({ + type: 'mouseMove', + index, + indexDiffSign: getIndexDiffSign(index, states.lastHoveredIndex), + }); };
79-86: Fix NaN risk on mouseDown and avoid duplicate lookupsSingle parse, validate, then dispatch.
- const handleMouseDown = (e: React.MouseEvent<HTMLSpanElement>) => { + const handleMouseDown = (e: MouseEvent<HTMLSpanElement>) => { e.preventDefault(); - dispatch({ - type: 'mouseDown', - index: getSelectedIndex(e), - isSelectionMode: !states.isSelectedStates[getSelectedIndex(e)], - }); + const index = getSelectedIndex(e); + if (index < 0) return; + dispatch({ + type: 'mouseDown', + index, + isSelectionMode: !states.isSelectedStates[index], + }); };
🧹 Nitpick comments (4)
src/pages/user/Apply/type/apply.ts (2)
71-80: NarrowpreviousIndexDiffSignto a sign unionConstrain to -1 | 0 | 1 for type-safety and to match
getIndexDiffSign.+export type IndexDiffSign = -1 | 0 | 1; export type DragState = { startIndex: number; lastHoveredIndex: number; currentSelectedIndex: number; isSelectionMode: boolean; isSelectedStates: boolean[]; isMouseDown: boolean; isDragging: boolean; - previousIndexDiffSign: null | number; + previousIndexDiffSign: null | IndexDiffSign; };
82-85: ConstrainindexDiffSignand addresetaction to support re-initKeeps action payloads precise and enables state reset when date/slots change.
-export type DragAction = - | { type: 'mouseDown'; index: number; isSelectionMode: boolean } - | { type: 'mouseMove'; index: number; indexDiffSign: number } - | { type: 'mouseUp' }; +export type DragAction = + | { type: 'mouseDown'; index: number; isSelectionMode: boolean } + | { type: 'mouseMove'; index: number; indexDiffSign: IndexDiffSign } + | { type: 'mouseUp' } + | { type: 'reset'; statesLength: number };src/pages/user/Apply/domain/drag.ts (2)
1-1: Import the sign type for consistencyAligns with DragAction/State narrowed types.
-import type { DragState } from '../type/apply'; +import type { DragState, IndexDiffSign } from '../type/apply';
9-17: NarrowidxDiffSignand return type fields to the sign unionImproves correctness guarantees.
export const updateDragState = ( state: DragState, currentIdx: number, - idxDiffSign: number, + idxDiffSign: IndexDiffSign, ): UpdatedDragState => { let newStartIndex = state.startIndex; let newIsSelectionMode = state.isSelectionMode; - let newPreviousIndexDiffSign = state.previousIndexDiffSign; + let newPreviousIndexDiffSign: IndexDiffSign | null = state.previousIndexDiffSign;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pages/user/Apply/components/ApplicationForm/InterviewScheduleSelector.tsx(2 hunks)src/pages/user/Apply/domain/drag.ts(1 hunks)src/pages/user/Apply/hook/useDragSelection.ts(1 hunks)src/pages/user/Apply/hook/useFormDataUpdate.ts(1 hunks)src/pages/user/Apply/type/apply.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/user/Apply/hook/useFormDataUpdate.ts
- src/pages/user/Apply/components/ApplicationForm/InterviewScheduleSelector.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/user/Apply/domain/drag.ts (1)
src/pages/user/Apply/type/apply.ts (1)
DragState(71-80)
src/pages/user/Apply/hook/useDragSelection.ts (6)
src/pages/user/Apply/type/apply.ts (2)
DragState(71-80)DragAction(82-85)src/pages/user/Apply/domain/drag.ts (1)
updateDragState(9-33)src/pages/user/Apply/utils/drag.ts (1)
updateSelectedState(1-13)src/pages/user/Apply/hook/useFormDataUpdate.ts (1)
useInterviewScheduleUpdater(6-19)src/pages/user/Apply/constant/initialDragState.ts (1)
generateInitialDragState(3-14)src/pages/user/Apply/utils/math.ts (1)
getIndexDiffSign(13-15)
| function getSelectedIndex(e: React.MouseEvent<HTMLSpanElement>) { | ||
| return Number(e.currentTarget.dataset.index); | ||
| } |
There was a problem hiding this comment.
Guard invalid index in getSelectedIndex
Avoids NaN array indexing and dispatch with bad payloads.
-function getSelectedIndex(e: React.MouseEvent<HTMLSpanElement>) {
- return Number(e.currentTarget.dataset.index);
-}
+function getSelectedIndex(e: MouseEvent<HTMLSpanElement>) {
+ const raw = e.currentTarget.dataset.index;
+ const idx = raw === undefined ? NaN : Number.parseInt(raw, 10);
+ return Number.isFinite(idx) ? idx : -1;
+}📝 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.
| function getSelectedIndex(e: React.MouseEvent<HTMLSpanElement>) { | |
| return Number(e.currentTarget.dataset.index); | |
| } | |
| function getSelectedIndex(e: MouseEvent<HTMLSpanElement>) { | |
| const raw = e.currentTarget.dataset.index; | |
| const idx = raw === undefined ? NaN : Number.parseInt(raw, 10); | |
| return Number.isFinite(idx) ? idx : -1; | |
| } |
🤖 Prompt for AI Agents
In src/pages/user/Apply/hook/useDragSelection.ts around lines 10–12,
getSelectedIndex currently does Number(...) on dataset.index which can produce
NaN and lead to invalid array indexing/dispatch; change it to parse the value
(e.g. const idx = parseInt(e.currentTarget.dataset.index ?? "", 10)), validate
with Number.isFinite(idx) / !Number.isNaN(idx) (or Number.isInteger), and return
a safe sentinel (null or -1) when invalid; also update any callers to check for
the sentinel before indexing or dispatching to avoid using NaN as an index.
| function dragReducer(state: DragState, action: DragAction) { | ||
| switch (action.type) { | ||
| case 'mouseDown': { | ||
| const newSelectedStates: boolean[] = [...state.isSelectedStates]; | ||
| newSelectedStates[action.index] = action.isSelectionMode; | ||
|
|
||
| return { | ||
| ...state, | ||
| startIndex: action.index, | ||
| currentSelectedIndex: action.index, | ||
| lastHoveredIndex: action.index, | ||
| isSelectionMode: action.isSelectionMode, | ||
| isSelectedStates: newSelectedStates, | ||
| isMouseDown: true, | ||
| previousIndexDiffSign: null, | ||
| }; | ||
| } | ||
| case 'mouseMove': { | ||
| const currentIndex = action.index; | ||
| const indexDiffSign = action.indexDiffSign; | ||
|
|
||
| const { newStartIndex, newIsSelectionMode, newPreviousIndexDiffSign } = updateDragState( | ||
| state, | ||
| currentIndex, | ||
| indexDiffSign, | ||
| ); | ||
|
|
||
| const newSelectedStates = updateSelectedState( | ||
| state.isSelectedStates, | ||
| newStartIndex, | ||
| currentIndex, | ||
| newIsSelectionMode, | ||
| ); | ||
|
|
||
| return { | ||
| ...state, | ||
| isDragging: true, | ||
| currentSelectedIndex: currentIndex, | ||
| lastHoveredIndex: currentIndex, | ||
| isSelectedStates: newSelectedStates, | ||
| isSelectionMode: newIsSelectionMode, | ||
| startIndex: newStartIndex, | ||
| previousIndexDiffSign: newPreviousIndexDiffSign, | ||
| }; | ||
| } | ||
| case 'mouseUp': { | ||
| return { | ||
| ...state, | ||
| isMouseDown: false, | ||
| isDragging: false, | ||
| }; | ||
| } | ||
| setPrevDiffSign(currentSign); | ||
| default: | ||
| return state; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add reset action handling to reducer
Supports safe re-init when date/slots change.
function dragReducer(state: DragState, action: DragAction) {
switch (action.type) {
+ case 'reset': {
+ return generateInitialDragState(action.statesLength);
+ }📝 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.
| function dragReducer(state: DragState, action: DragAction) { | |
| switch (action.type) { | |
| case 'mouseDown': { | |
| const newSelectedStates: boolean[] = [...state.isSelectedStates]; | |
| newSelectedStates[action.index] = action.isSelectionMode; | |
| return { | |
| ...state, | |
| startIndex: action.index, | |
| currentSelectedIndex: action.index, | |
| lastHoveredIndex: action.index, | |
| isSelectionMode: action.isSelectionMode, | |
| isSelectedStates: newSelectedStates, | |
| isMouseDown: true, | |
| previousIndexDiffSign: null, | |
| }; | |
| } | |
| case 'mouseMove': { | |
| const currentIndex = action.index; | |
| const indexDiffSign = action.indexDiffSign; | |
| const { newStartIndex, newIsSelectionMode, newPreviousIndexDiffSign } = updateDragState( | |
| state, | |
| currentIndex, | |
| indexDiffSign, | |
| ); | |
| const newSelectedStates = updateSelectedState( | |
| state.isSelectedStates, | |
| newStartIndex, | |
| currentIndex, | |
| newIsSelectionMode, | |
| ); | |
| return { | |
| ...state, | |
| isDragging: true, | |
| currentSelectedIndex: currentIndex, | |
| lastHoveredIndex: currentIndex, | |
| isSelectedStates: newSelectedStates, | |
| isSelectionMode: newIsSelectionMode, | |
| startIndex: newStartIndex, | |
| previousIndexDiffSign: newPreviousIndexDiffSign, | |
| }; | |
| } | |
| case 'mouseUp': { | |
| return { | |
| ...state, | |
| isMouseDown: false, | |
| isDragging: false, | |
| }; | |
| } | |
| setPrevDiffSign(currentSign); | |
| default: | |
| return state; | |
| } | |
| } | |
| function dragReducer(state: DragState, action: DragAction) { | |
| switch (action.type) { | |
| case 'reset': { | |
| return generateInitialDragState(action.statesLength); | |
| } | |
| case 'mouseDown': { | |
| const newSelectedStates: boolean[] = [...state.isSelectedStates]; | |
| newSelectedStates[action.index] = action.isSelectionMode; | |
| return { | |
| ...state, | |
| startIndex: action.index, | |
| currentSelectedIndex: action.index, | |
| lastHoveredIndex: action.index, | |
| isSelectionMode: action.isSelectionMode, | |
| isSelectedStates: newSelectedStates, | |
| isMouseDown: true, | |
| previousIndexDiffSign: null, | |
| }; | |
| } | |
| case 'mouseMove': { | |
| const currentIndex = action.index; | |
| const indexDiffSign = action.indexDiffSign; | |
| const { newStartIndex, newIsSelectionMode, newPreviousIndexDiffSign } = updateDragState( | |
| state, | |
| currentIndex, | |
| indexDiffSign, | |
| ); | |
| const newSelectedStates = updateSelectedState( | |
| state.isSelectedStates, | |
| newStartIndex, | |
| currentIndex, | |
| newIsSelectionMode, | |
| ); | |
| return { | |
| ...state, | |
| isDragging: true, | |
| currentSelectedIndex: currentIndex, | |
| lastHoveredIndex: currentIndex, | |
| isSelectedStates: newSelectedStates, | |
| isSelectionMode: newIsSelectionMode, | |
| startIndex: newStartIndex, | |
| previousIndexDiffSign: newPreviousIndexDiffSign, | |
| }; | |
| } | |
| case 'mouseUp': { | |
| return { | |
| ...state, | |
| isMouseDown: false, | |
| isDragging: false, | |
| }; | |
| } | |
| default: | |
| return state; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/pages/user/Apply/hook/useDragSelection.ts around lines 14 to 69, the
reducer lacks a 'reset' action to safely reinitialize drag state when dates or
slots change; add a 'case "reset"' that returns a fresh state (either from
action.payload or by constructing an initial state) resetting startIndex,
currentSelectedIndex, lastHoveredIndex to null,
isSelectionMode/isMouseDown/isDragging to false, previousIndexDiffSign to null,
and isSelectedStates to the new selection array provided in the payload (or
recomputed), ensuring you return a new object (no in-place mutation) so the hook
fully re-initializes on date/slot changes.
| export function useDragSelection(date: string, timeIntervalArray: [string, string][]) { | ||
| const { updateInterviewSchedule } = useInterviewScheduleUpdater(date, timeIntervalArray); | ||
|
|
||
| const newValue = [...selectedTime]; | ||
| newValue[Number(startIndex.current)] = mode.current; | ||
| const [states, dispatch] = useReducer( | ||
| dragReducer, | ||
| generateInitialDragState(timeIntervalArray.length), | ||
| ); |
There was a problem hiding this comment.
Reset reducer when date or slots length change
Prevents length mismatch and out-of-bounds updates after switching days or slot lists.
export function useDragSelection(date: string, timeIntervalArray: [string, string][]) {
const { updateInterviewSchedule } = useInterviewScheduleUpdater(date, timeIntervalArray);
const [states, dispatch] = useReducer(
dragReducer,
generateInitialDragState(timeIntervalArray.length),
);
+
+ useEffect(() => {
+ dispatch({ type: 'reset', statesLength: timeIntervalArray.length });
+ }, [date, timeIntervalArray.length]);📝 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.
| export function useDragSelection(date: string, timeIntervalArray: [string, string][]) { | |
| const { updateInterviewSchedule } = useInterviewScheduleUpdater(date, timeIntervalArray); | |
| const newValue = [...selectedTime]; | |
| newValue[Number(startIndex.current)] = mode.current; | |
| const [states, dispatch] = useReducer( | |
| dragReducer, | |
| generateInitialDragState(timeIntervalArray.length), | |
| ); | |
| export function useDragSelection(date: string, timeIntervalArray: [string, string][]) { | |
| const { updateInterviewSchedule } = useInterviewScheduleUpdater(date, timeIntervalArray); | |
| const [states, dispatch] = useReducer( | |
| dragReducer, | |
| generateInitialDragState(timeIntervalArray.length), | |
| ); | |
| useEffect(() => { | |
| dispatch({ type: 'reset', statesLength: timeIntervalArray.length }); | |
| }, [date, timeIntervalArray.length]); | |
| // …rest of your hook… | |
| } |
🤖 Prompt for AI Agents
In src/pages/user/Apply/hook/useDragSelection.ts around lines 71-77, the reducer
state can get out-of-sync when the date or the number of time slots changes; add
logic to reset the reducer whenever date or timeIntervalArray.length changes.
Implement a RESET (or REINITIALIZE) action in dragReducer that replaces state
with generateInitialDragState(timeIntervalArray.length), then add a useEffect
that watches [date, timeIntervalArray.length] and dispatches that RESET action
with the freshly generated initial state; alternatively, if dragReducer already
supports a reset action name, dispatch that action from the effect. Ensure the
effect runs on mount and when those dependencies change so state length always
matches slots.
There was a problem hiding this comment.
updateDragState랑 utils의 updateSelectedState 함수들은 useDragSelection훅 파일 내부에만 사용이 될 것 같고 함께 두면 응집도도 높아지고 나중에 수정할 때도 한 파일에 모여 있어 코드를 찾기 쉬울 것 같은데 분리하신 이유가 무엇인지 궁금합니다!
There was a problem hiding this comment.
말씀하신대로 훅 내부에서만 사용되는 함수이고, 같이 두면 찾기 편한 측면은 분명히 존재하는 것 같습니다!
그런데 제가 생각하기에는 응집도는 사용되는 로직들이 함께 위치한다고 높아지는 것은 아니고 수행하는 기능 측면에서 생각하는게 맞다고 생각했습니다.
updateSelectedState는 배열, 시작인덱스, 종료 인덱스만 매개변수로 넣으면 상태 변환된 배열을 출력해주는 함수이고, 드래그 뿐만 아니라 배열의 특정 범위를 바꾸는 영역에서 추가로 사용할 수 있으므로 util 영역이 적절하다고 판단했고,
useDragSelection은 "드래그 방향이 바뀌면 선택/해제를 반대로 한다"는 면접 일정을 선택할 때만 반영되는 어떤 규칙이 반영된 부분이라 util이 아닌 곳에 두어야 한다고 판단했습니다. 지난번에 멘토님이 보내주신 FSD 관련 구조 뿐만 아니라 다수의 프로젝트에서 도메인/비즈니스 로직 등을 따로 관리하는 게 일반적이라 현재 프로젝트에도 domain 폴더가 적합해 보여서 추가했습니다.
결과적으로 useDragSelection 훅 내부에서 util 함수와 domain 함수를 불러서 사용해도 기능 측면에서 응집도는 유지된다고 생각했습니다.
| const updateInterviewSchedule = (isSelectedStates: boolean[]) => { | ||
| const selectedTimes = convertSelectionToTimeInterval(isSelectedStates, timeSlotsArray); | ||
| const mergedTimes = mergeContinuousTimeInterval(selectedTimes); | ||
| const currentSchedules: PostInterviewSchedule[] = getValues('selectedInterviewSchedule') || []; | ||
|
|
||
| const updatedSchedules = updateSchedule(currentSchedules, date, mergedTimes); | ||
| setValue('selectedInterviewSchedule', updatedSchedules); | ||
| }; | ||
|
|
||
| return { updateInterviewSchedule }; | ||
| } |
There was a problem hiding this comment.
updateInterviewSchedule을 [date, timeSlotsArray, getValues, setValue] 의존성 배열로 useCallback으로 감싸는 것도 괜찮을 것 같은데 어떠신가요?
There was a problem hiding this comment.
이 부분이 조금 헷갈리는데, useCallback으로 내부를 감싸도
export const InterviewScheduleSelector = ({ availableTime, date }: InterviewSchedule) => {
const timeSlotsArray: [string, string][] = getTimeSlotsArray(availableTime);
const { handleMouseDown, handleMouseMove, handleMouseUp, states } = useDragSelection(
date,
timeSlotsArray,
);
상위 component가 렌더링 되면 -> hook이 계속 호출되는 구조 -> useCallback에 기대했던 역할을 수행 불가라고 생각했는데,
그래서 리팩토링 과정에서 상위 컴포넌트의 props를 오히려 memo를 통해 매개변수로 넣어야되지 않을까 생각했는데 어떤 게 맞을까요?
There was a problem hiding this comment.
오 그렇네요!
말씀하신 대로 상위 컴포넌트가 리렌더링되면서 매번 새로운 props를 생성하니까 useCallback을 해도 효과가 별로 없네요
먼저 props나 timeSlotsArray를 useMemo로 고정한 뒤 useCallback을 함께 적용하는 게 더 나은 방법인 것 같아요
3 participants
#️⃣연관된 이슈
📝작업 내용
feat:
refactor:
버그 수정:
스타일 변경:
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor