Fix#358 필드 선택 시 다음 필드를 자동으로 포커스 및 클릭하도록 수정 #359
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughImplements ref-forwarding in SelectField and adds scroll-aware focus progression across examDate → schoolName → lunch fields in the Apply form. On each selection, downstream fields reset and the next field is focused/opened, ensuring visibility within horizontally scrollable containers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant AF as ApplyForm (SelectExamField)
participant SF as SelectField
participant SA as scrollAfterSelect
participant DOM as Scroll Container (.overflow-x-auto)
U->>SF: Select Area
SF-->>AF: onValueChange(area)
AF->>AF: Reset examDate, schoolName, lunch
AF->>SA: scrollAfterSelect(examDateRef)
SA->>DOM: Ensure visibility (scroll if needed)
SA-->>SF: Focus + click examDate
U->>SF: Select examDate
SF-->>AF: onValueChange(examDate)
AF->>AF: Reset schoolName, lunch
AF->>SA: scrollAfterSelect(schoolNameRef)
SA->>DOM: Ensure visibility (scroll if needed)
SA-->>SF: Focus + click schoolName
U->>SF: Select schoolName
SF-->>AF: onValueChange(schoolName)
AF->>AF: Reset lunch
AF->>SA: scrollAfterSelect(lunchRef)
SA->>DOM: Ensure visibility (scroll if needed)
SA-->>SF: Focus + click lunch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 추가 및 변경된 파일총 2개 파일 변경 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mosu-app/src/shared/components/SelectField/SelectField.tsx (1)
15-27: Refs are not forwarded — current implementation will not receive caller refsReact strips the
refprop frompropsand passes it as a second argument only when the component is wrapped withReact.forwardRef. As written:
- Adding
ref?: …toSelectFieldProps(Line 26) does nothing at runtime.- Destructuring
reffromprops(Line 40) will always yieldundefined.- Passing that to
<SelectTrigger ref={ref} …>(Line 71) therefore never connects the caller’s ref to the trigger.Net effect: all uses like
<SelectField ref={examDateRef} …/>in SelectExamField won’t work; focus/click chaining won’t fire. Fix by convertingSelectFieldto a forwardRef and removingreffrom the props interface.Apply this diff:
@@ -export interface SelectFieldProps<T extends FieldValues> extends React.ComponentProps<typeof SelectPrimitive.Root> { +export interface SelectFieldProps<T extends FieldValues> extends React.ComponentProps<typeof SelectPrimitive.Root> { id: string; placeholder: string; triggerClassName?: string; options?: Array<Option>; name?: FieldPath<T>; register?: UseFormRegister<T>; watch?: UseFormWatch<T>; error?: string | boolean; shouldDisplayErrorMessage?: boolean; onValueChange?: (value: string, metadata?: unknown) => void; - ref?: React.Ref<React.ComponentRef<typeof SelectTrigger>>; } @@ -export const SelectField = <T extends FieldValues>({ - id, - triggerClassName, - placeholder, - options, - name, - register, - watch: externalWatch, - error, - shouldDisplayErrorMessage = false, - onValueChange, - ref, - ...props -}: SelectFieldProps<T>) => { +type TriggerEl = React.ComponentRef<typeof SelectTrigger>; + +const SelectFieldInner = <T extends FieldValues>( + { + id, + triggerClassName, + placeholder, + options, + name, + register, + watch: externalWatch, + error, + shouldDisplayErrorMessage = false, + onValueChange, + ...props + }: SelectFieldProps<T>, + ref: React.Ref<TriggerEl> +) => { @@ - return ( - <Select {...props} onValueChange={onValueChangeHandler} name={registerProps.name} value={currentValue}> - <SelectTrigger ref={ref} className={cn(error ? "border-red-400" : "", triggerClassName)}> + return ( + <Select {...props} onValueChange={onValueChangeHandler} name={registerProps.name} value={currentValue}> + <SelectTrigger ref={ref} className={cn(error ? "border-red-400" : "", triggerClassName)}> <SelectValue placeholder={placeholder} /> </SelectTrigger> <SelectContent id={id}> @@ - </Select> - ); -}; + </Select> + ); +}; + +export const SelectField = forwardRef(SelectFieldInner) as < + T extends FieldValues +>( + p: SelectFieldProps<T> & { ref?: React.Ref<TriggerEl> } +) => JSX.Element;Notes:
- This preserves the generic
Tfor form typing while enabling true ref forwarding to the internalSelectTrigger.- No external API change for callers; they can keep using
ref={...}.Also applies to: 40-42, 71-71
♻️ Duplicate comments (1)
mosu-app/src/features/apply/ui/SelectExamField.tsx (1)
27-29: Downstream refs won’t be populated until SelectField forwards the refThe new refs here are correct, but they currently receive
nullbecauseSelectFielddoesn’t forward refs (see prior comment). After applying the forwardRef fix, these lines will work as intended.Also applies to: 108-108, 125-125, 143-143
🧹 Nitpick comments (5)
mosu-app/src/shared/components/SelectField/SelectField.tsx (1)
3-3: ImportforwardReffor proper ref forwardingYou’ll need
forwardRefhere once we fix the ref wiring below.Apply this diff:
-import { useCallback, useMemo } from "react"; +import { useCallback, useMemo, forwardRef } from "react";mosu-app/src/features/apply/ui/SelectExamField.tsx (4)
37-71: Simplify sequencing; avoid nested rAF + setTimeout(0) and make scroll delay configurableThe double-scheduling (rAF + setTimeout 0) is redundant and the hard-coded 200ms can be brittle across devices. Recommend:
- Use a single rAF for layout stability after state updates.
- Hoist the click-delay into a constant for easier tuning.
- Guard early, and reuse locals to reduce repeated
ref.currentlookups.Apply this diff:
- const scrollAfterSelect = (ref: React.RefObject<HTMLButtonElement>) => { - requestAnimationFrame(() => { - setTimeout(() => { - if (ref.current) { - ref.current.focus({ preventScroll: true }); - - const container = ref.current.closest(".overflow-x-auto"); - const rect = ref.current.getBoundingClientRect(); - const containerRect = container?.getBoundingClientRect(); - - let needsScroll = false; - if (containerRect) { - needsScroll = rect.left < containerRect.left || rect.right > containerRect.right; - } else { - needsScroll = rect.left < 0 || rect.right > window.innerWidth; - } - - if (needsScroll) { - ref.current.scrollIntoView({ - behavior: "smooth", - block: "nearest", - inline: "center", - }); - - setTimeout(() => { - ref.current?.click(); - }, 200); - } else { - ref.current.click(); - } - } - }, 0); - }); - }; + const SCROLL_CLICK_DELAY_MS = 160; + const scrollAfterSelect = (ref: React.RefObject<HTMLButtonElement>) => { + requestAnimationFrame(() => { + const el = ref.current; + if (!el) return; + + el.focus({ preventScroll: true }); + + const container = el.closest(".overflow-x-auto") as HTMLElement | null; + const rect = el.getBoundingClientRect(); + const containerRect = container?.getBoundingClientRect(); + + const needsScroll = containerRect + ? rect.left < containerRect.left || rect.right > containerRect.right + : rect.left < 0 || rect.right > window.innerWidth; + + if (needsScroll) { + el.scrollIntoView({ behavior: "smooth", block: "nearest", inline: "center" }); + window.setTimeout(() => el?.click(), SCROLL_CLICK_DELAY_MS); + } else { + el.click(); + } + }); + };
97-103: Option loading race: consider deferring open until options are readyAfter changing
area, you immediately try to focus/openexamDate. IfuseGetExamByExamAreahasn’t populated options yet, the dropdown will open empty. Consider:
- Opening only when
!isLoading && !isPlaceholderData, or- Opening immediately but re-opening once data arrives.
If you choose to defer open:
onValueChange={() => { resetField(`examApplication.${index}.examDate`); resetField(`examApplication.${index}.schoolName`); resetField(`examApplication.${index}.lunch`); - scrollAfterSelect(examDateRef as React.RefObject<HTMLButtonElement>); + if (!isLoading && !isPlaceholderData) { + scrollAfterSelect(examDateRef as React.RefObject<HTMLButtonElement>); + } }}And add an effect to open when data flips ready (optional).
132-137: Guard against invalid metadata before setting examId
metadatais typed asunknown.Number(metadata)can yieldNaN, resulting in an invalidexamIdbeing written to the form. Add a simple guard.Apply this diff:
- onValueChange={(_, metadata) => { - const examId = Number(metadata); - setValue(`examApplication.${index}.examId`, examId); + onValueChange={(_, metadata) => { + const examId = typeof metadata === "number" ? metadata : Number(metadata); + if (Number.isFinite(examId)) { + setValue(`examApplication.${index}.examId`, examId); + } resetField(`examApplication.${index}.lunch`); scrollAfterSelect(lunchRef as React.RefObject<HTMLButtonElement>); }}
88-90: Minor UX: keep RemoveButton tabbable but out of the auto-advance pathWith focus auto-advancing across fields, it’s easy to skip the remove control. If this fieldset is keyboard-heavy, consider:
- Giving the RemoveButton a consistent placement and a tooltip/label, and
- Ensuring it’s reachable via Shift+Tab from the first select.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
mosu-app/src/features/apply/ui/SelectExamField.tsx(5 hunks)mosu-app/src/shared/components/SelectField/SelectField.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mosu-app/src/features/apply/ui/SelectExamField.tsx (1)
mosu-app/src/features/apply/services/getExamByExamArea.ts (1)
useGetExamByExamArea(34-41)
mosu-app/src/shared/components/SelectField/SelectField.tsx (1)
mosu-app/src/shared/components/SelectField/index.ts (1)
SelectFieldProps(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit & Integration Tests
- GitHub Check: Run DangerJS
🔇 Additional comments (1)
mosu-app/src/shared/components/SelectField/SelectField.tsx (1)
70-70: Controlled vs. uncontrolled value — confirmcurrentValueis always defined or intentionally undefined
value={currentValue}will make theSelectcontrolled whencurrentValueis a string and controlled-with-undefined otherwise. Verify thatexternalWatch(name)returnsundefinedinitially and does not flip betweenundefinedand a value in the same render pass; otherwise you may see React warnings about switching between controlled/uncontrolled inputs. If needed, coalesce to""or explicitly passdefaultValue.
📚 Storybook이 Chromatic에 배포되었습니다!
|
|
25/8/23 |
✅ Linked Issue
🔍 What I did
SelectField.tsx에 ref props 추가requestAnimationFrame과setTimeout으로 다음 필드를 자동으로 포커스 및 클릭하도록 추가Summary by CodeRabbit