-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor/#94 SearchPage 데이터 흐름 개선 #95
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
Conversation
- 부모 컴포넌트에서 관리하던 상태를 SearchPage로 이동 - 부모 컴포넌트의 불필요한 리렌더링 제거
WalkthroughSearchPage가 내부 디바운스된 fetch 흐름으로 전환되어 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchPage
participant DebouncedHook as useDebouncedFetch
participant API as searchFunc (external)
User->>SearchPage: 입력(키 입력)
SearchPage->>DebouncedHook: trigger(inputValue)
DebouncedHook-->>DebouncedHook: 타이머 대기(예: 300ms)
DebouncedHook->>API: 호출(searchFunc(inputValue))
API-->>DebouncedHook: Promise<BasePlace[]> 반환
DebouncedHook-->>SearchPage: data(places) 반환
SearchPage-->>User: 결과 렌더링 (목록, 스피너/isSelecting 업데이트)
User->>SearchPage: 항목 클릭
SearchPage->>SearchPage: isSelecting = true
SearchPage->>Caller: onSelectPlace 호출
SearchPage->>SearchPage: isSelecting = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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 |
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
🤖 Fix all issues with AI agents
In `@apps/web/app/_components/SearchPage/SearchPage.tsx`:
- Line 51: setIsLoading is set to true in several handlers (notably
onSelectPlace) but never reset to false; wrap the async logic in those handlers
(e.g., onSelectPlace and the other places calling setIsLoading(true)) with
try/finally and call setIsLoading(false) in the finally block (or ensure it is
set to false after awaiting navigation failure), so the spinner is reliably
cleared even if component doesn't unmount.
- Around line 49-59: handleInputChange currently only calls setPlaces when
value.length > 0, so clearing the input leaves previous search results in
places; update handleInputChange to call setPlaces([]) (or the appropriate empty
value your useDebouncedFetch expects) when value is empty so places is reset
when the user clears the input; modify the handleInputChange function to branch
on value.length and call setPlaces(value) when non-empty and setPlaces([]) when
empty, referencing handleInputChange, setPlaces and places.
🧹 Nitpick comments (4)
apps/web/app/_hooks/useDebouncedFetch.ts (3)
29-44:fetcher의존성으로 인한 잠재적 문제
fetcher가useCallback의존성 배열에 포함되어 있어, 호출 측에서fetcher가 매 렌더링마다 새 참조를 생성하면trigger함수도 매번 재생성됩니다. 이로 인해 불필요한 리렌더링이나 stale closure 문제가 발생할 수 있습니다.호출 측에서
fetcher를useCallback으로 감싸거나, 훅 내부에서useRef로 최신fetcher를 추적하는 방식을 고려해 보세요.♻️ useRef를 활용한 대안
export const useDebouncedFetch = <T, P>( fetcher: (params: P) => Promise<T[]>, delay: number = 300, ) => { const [data, setData] = useState<T[]>([]) const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null) + const fetcherRef = useRef(fetcher) + fetcherRef.current = fetcher const trigger = useCallback( (params: P) => { if (timeoutRef.current) clearTimeout(timeoutRef.current) timeoutRef.current = setTimeout(async () => { try { - const result = await fetcher(params) + const result = await fetcherRef.current(params) setData(result) } catch (error) { console.error('Debounced fetch failed:', error) setData([]) } }, delay) }, - [fetcher, delay], + [delay], )
33-40: Race condition 가능성빠르게 연속 입력 후 느린 네트워크 환경에서, 이전 요청이 최신 요청보다 늦게 완료되면 오래된 데이터가 표시될 수 있습니다.
AbortController를 사용하여 이전 요청을 취소하거나, 요청 ID를 통해 최신 응답만 반영하는 방식을 고려해 보세요.
22-54: 입력 초기화 시 데이터 클리어 메커니즘 부재입력값이 비워졌을 때
places상태를 초기화하는 로직이 훅 내부에 없습니다.SearchPage.tsx의handleInputChange에서value.length > 0일 때만setPlaces를 호출하므로, 입력을 지워도 이전 검색 결과가 그대로 남아있게 됩니다.데이터를 명시적으로 초기화할 수 있는
clear함수를 반환하거나, 빈 문자열 입력 시 데이터를 자동으로 초기화하는 로직 추가를 권장합니다.♻️ clear 함수 추가 예시
+ const clear = useCallback(() => setData([]), []) + - return [data, trigger] as const + return [data, trigger, clear] as constapps/web/app/places/search/page.tsx (1)
11-18:handleSearch에useCallback적용 권장
handleSearch가 컴포넌트 내부에서 인라인으로 정의되어 매 렌더링마다 새 함수 참조가 생성됩니다.useDebouncedFetch의useCallback의존성에fetcher가 포함되어 있어, 불필요한trigger함수 재생성이 발생할 수 있습니다.
PlaceSearch.tsx에서는useCallback을 사용한 것과 일관성을 맞추는 것이 좋습니다.♻️ useCallback 적용
+'use client' + +import { useCallback } from 'react' import { useRouter } from 'next/navigation' import { SearchPage } from '@/_components/SearchPage' import { getPlacesBySearch } from '@/_apis/services/place' import { CLIENT_PATH } from '@/_constants/path' const Page = () => { const { replace } = useRouter() - const handleSearch = async (query: string) => { + const handleSearch = useCallback(async (query: string) => { const result = await getPlacesBySearch(query) return result.map((place) => ({ id: place.placeId, name: place.placeName, address: place.address, })) - } + }, [])
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/_components/SearchPage/SearchPage.tsxapps/web/app/_hooks/useDebouncedFetch.tsapps/web/app/_hooks/useSearch.tsapps/web/app/places/new/_components/Step/PlaceSearch/PlaceSearch.tsxapps/web/app/places/search/page.tsx
💤 Files with no reviewable changes (1)
- apps/web/app/_hooks/useSearch.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/app/_hooks/useDebouncedFetch.ts (1)
apps/web/app/_hooks/useSearch.ts (4)
T(23-53)params(28-41)timeoutRef(46-50)timeoutRef(47-49)
apps/web/app/places/search/page.tsx (3)
apps/web/app/_apis/services/place.ts (1)
getPlacesBySearch(58-63)apps/web/app/_components/SearchPage/SearchPage.tsx (1)
SearchPage(43-96)apps/web/app/requests/_components/RequestPlacesList/RequestPlacesList.tsx (1)
place(17-19)
apps/web/app/_components/SearchPage/SearchPage.tsx (1)
apps/web/app/_hooks/useDebouncedFetch.ts (1)
useDebouncedFetch(22-54)
🔇 Additional comments (5)
apps/web/app/places/search/page.tsx (1)
20-27: LGTM!
SearchPage컴포넌트 사용이 적절합니다.useBackHandler={true}추가와onSelectPlace콜백 구현이 잘 되어 있습니다.apps/web/app/places/new/_components/Step/PlaceSearch/PlaceSearch.tsx (2)
15-29: LGTM!
useCallback이 올바르게 적용되었고, 의존성 배열에campus만 포함된 것이 적절합니다.BasePlace타입에 맞는 매핑 로직도 깔끔합니다.
31-39: LGTM!
SearchPage컴포넌트 사용과onSelectPlace콜백 구현이 잘 되어 있습니다.apps/web/app/_components/SearchPage/SearchPage.tsx (2)
9-13: LGTM!
BasePlace타입이 잘 정의되어 있고, 재사용 가능하도록 export되어 있습니다.
43-48: LGTM!기본 placeholder 값 설정과 props 구조가 잘 정리되어 있습니다.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const [places, setPlaces] = useDebouncedFetch(searchFunc) | ||
| const [inputValue, setInputValue] = useState('') | ||
| const [isLoading, setIsLoading] = useState(false) | ||
|
|
||
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value | ||
| setInputValue(value) | ||
| if (value.length > 0) { | ||
| searchFunc(value) | ||
| setPlaces(value) | ||
| } | ||
| } |
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.
입력값 초기화 시 검색 결과가 유지되는 문제
value.length > 0일 때만 setPlaces를 호출하므로, 사용자가 입력을 모두 지워도 이전 검색 결과(places)가 그대로 남아있습니다. inputValue && places.map(...) 조건으로 렌더링은 숨겨지지만, 상태는 남아있어 다시 입력 시 잠깐 이전 결과가 보일 수 있습니다.
입력이 비워질 때 places를 초기화하는 것이 더 깔끔합니다.
🐛 수정 제안
const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const value = e.target.value
setInputValue(value)
if (value.length > 0) {
setPlaces(value)
+ } else {
+ // places 초기화 로직 필요 (useDebouncedFetch에 clear 함수 추가 필요)
}
}🤖 Prompt for AI Agents
In `@apps/web/app/_components/SearchPage/SearchPage.tsx` around lines 49 - 59,
handleInputChange currently only calls setPlaces when value.length > 0, so
clearing the input leaves previous search results in places; update
handleInputChange to call setPlaces([]) (or the appropriate empty value your
useDebouncedFetch expects) when value is empty so places is reset when the user
clears the input; modify the handleInputChange function to branch on
value.length and call setPlaces(value) when non-empty and setPlaces([]) when
empty, referencing handleInputChange, setPlaces and places.
| }: Props) => { | ||
| const [places, setPlaces] = useDebouncedFetch(searchFunc) | ||
| const [inputValue, setInputValue] = useState('') | ||
| const [isLoading, setIsLoading] = useState(false) |
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.
isLoading 상태가 true로 설정된 후 초기화되지 않음
Line 87에서 setIsLoading(true)가 호출되지만, false로 되돌리는 로직이 없습니다. onSelectPlace가 페이지 이동(replace)을 수행하므로 컴포넌트가 언마운트되어 현재는 문제가 없지만, 향후 동작 변경 시 스피너가 계속 표시될 수 있습니다.
Also applies to: 63-64, 86-88
🤖 Prompt for AI Agents
In `@apps/web/app/_components/SearchPage/SearchPage.tsx` at line 51, setIsLoading
is set to true in several handlers (notably onSelectPlace) but never reset to
false; wrap the async logic in those handlers (e.g., onSelectPlace and the other
places calling setIsLoading(true)) with try/finally and call setIsLoading(false)
in the finally block (or ensure it is set to false after awaiting navigation
failure), so the spinner is reliably cleared even if component doesn't unmount.
- isLoading은 데이터 페칭(fetching)과 혼동될 여지가 있어 변경
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
🤖 Fix all issues with AI agents
In `@apps/web/app/_components/SearchPage/SearchPage.tsx`:
- Around line 86-90: The onClick handler currently calls setIsSelecting(true),
then onSelectPlace(place.id), then setIsSelecting(false) synchronously so React
batches updates and the spinner never appears; make the handler async and await
the async work: call setIsSelecting(true), await onSelectPlace(place.id) (or
ensure onSelectPlace returns a Promise), then setIsSelecting(false) in a finally
block to guarantee the spinner is cleared even on error; reference the onClick
handler, setIsSelecting, onSelectPlace, and place.id when updating the code.
♻️ Duplicate comments (1)
apps/web/app/_components/SearchPage/SearchPage.tsx (1)
53-59: 입력값 초기화 시 검색 결과가 남아있는 문제입력값이 비워질 때
places상태가 초기화되지 않습니다.inputValue &&조건으로 렌더링은 숨겨지지만, 상태는 유지되어 다시 입력 시 이전 결과가 잠깐 보일 수 있습니다.
useDebouncedFetch훅에clear또는reset함수를 추가하여 입력이 비워질 때 상태를 초기화하는 것을 권장합니다.♻️ useDebouncedFetch 훅 수정 제안
// useDebouncedFetch.ts const trigger = useCallback(...) + const clear = useCallback(() => { + if (timeoutRef.current) clearTimeout(timeoutRef.current) + setData([]) + }, []) - return [data, trigger] as const + return [data, trigger, clear] as const// SearchPage.tsx - const [places, setPlaces] = useDebouncedFetch(searchFunc) + const [places, setPlaces, clearPlaces] = useDebouncedFetch(searchFunc) const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>) => { const value = e.target.value setInputValue(value) if (value.length > 0) { setPlaces(value) + } else { + clearPlaces() } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/_components/SearchPage/SearchPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/_components/SearchPage/SearchPage.tsx (7)
apps/web/app/_hooks/useDebouncedFetch.ts (1)
useDebouncedFetch(22-54)apps/web/app/places/new/_components/Step/PlaceSearch/PlaceSearch.tsx (2)
Props(15-42)item(18-22)apps/web/app/places/search/page.tsx (1)
place(16-20)apps/web/app/requests/_components/RequestPlacesList/RequestPlacesList.tsx (1)
place(17-19)apps/web/app/map/_components/PlaceList/PlaceList.tsx (1)
place(31-33)apps/web/app/categories/[id]/_components/Places/Places.tsx (1)
place(24-31)apps/web/app/_components/SearchPage/SearchPlaceListItem.tsx (1)
inputValue(9-33)
🔇 Additional comments (3)
apps/web/app/_components/SearchPage/SearchPage.tsx (3)
43-51: LGTM!컴포넌트 초기화 로직이 깔끔합니다.
useDebouncedFetch훅 사용과 상태 관리가 적절합니다.
61-78: LGTM!JSX 구조가 깔끔하고, 조건부 렌더링 로직이 적절합니다.
15-20: SearchPage 시그니처 변경에 대한 호출자 호환성 확인원래 리뷰 코멘트에서 우려했던
PlaceSearch.tsx의 호환성 문제는 실제로 존재하지 않습니다. 현재 코드를 확인한 결과:
PlaceSearch.tsx의handleSearch는async (query: string) => Promise<{ id, name, address }[]>를 반환- SearchPage에 전달되는
searchFunc가 새로운 시그니처(Promise<BasePlace[]>반환)와 완벽하게 일치placesprop을 사용하지 않음 (올바른 패턴)따라서
SearchPage.tsx의 Props 변경사항이 호출자와 완전히 호환되므로, 추가 조치는 필요하지 않습니다.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| onClick={() => { | ||
| setIsLoading(true) | ||
| setIsSelecting(true) | ||
| onSelectPlace(place.id) | ||
| setIsSelecting(false) | ||
| }} |
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.
동기 호출로 인해 스피너가 표시되지 않음
setIsSelecting(true)와 setIsSelecting(false)가 동기적으로 연속 호출되어 React가 이를 배치 처리합니다. 결과적으로 스피너가 실제로 렌더링되지 않습니다.
onSelectPlace가 비동기 작업(예: 페이지 이동)을 수행한다면, async/await를 사용해야 합니다.
🐛 수정 제안
onClick={() => {
- setIsSelecting(true)
- onSelectPlace(place.id)
- setIsSelecting(false)
+ setIsSelecting(true)
+ // onSelectPlace가 Promise를 반환하지 않으면 스피너 로직 제거 고려
+ onSelectPlace(place.id)
+ // 페이지 이동 시 컴포넌트가 언마운트되므로 false 설정 불필요
}}또는 onSelectPlace가 Promise를 반환하도록 변경:
onClick={async () => {
setIsSelecting(true)
try {
await onSelectPlace(place.id)
} finally {
setIsSelecting(false)
}
}}🤖 Prompt for AI Agents
In `@apps/web/app/_components/SearchPage/SearchPage.tsx` around lines 86 - 90, The
onClick handler currently calls setIsSelecting(true), then
onSelectPlace(place.id), then setIsSelecting(false) synchronously so React
batches updates and the spinner never appears; make the handler async and await
the async work: call setIsSelecting(true), await onSelectPlace(place.id) (or
ensure onSelectPlace returns a Promise), then setIsSelecting(false) in a finally
block to guarantee the spinner is cleared even on error; reference the onClick
handler, setIsSelecting, onSelectPlace, and place.id when updating the code.
- 배치 처리 때문에 스피너가 실제로 렌더링되지 않음
#️⃣연관된 이슈
📝작업 내용
도메인에 종속적이었던
useSearch훅을 범용적으로 사용할 수 있도록 분리했습니다.검색 기능을 담당하는
SearchPage컴포넌트의 구조를 개선하여 데이터 흐름의 복잡도를 낮추고 렌더링 성능을 최적화했습니다.1. useSearch → useDebouncedFetch
도메인을 떼어버리고 기능 중심의 일반적인 이름으로 바꾸었습니다
또한 익숙한 useState처럼 사용할 수 있도록 반환 타입을 객체 형태에서 배열 형태로 변경했습니다.
추후 반환값이 isLoading, error 상태 등으로 확장된다면 객체 형태로 변경 고려 중 입니다.
2. 구조 개선 (Architecture)
Before : 데이터가 U턴하는 구조
입력 → 부모로 이벤트 전달
부모에서 API 호출 + 상태 업데이트
결과를 다시 props로 내려받아 렌더링
After
SearchPage를 “검색 기능을 자체적으로 완결하는 UI 컴포넌트” 로 만들었습니다.
“입력 → 요청 → 결과”가 한 파일 안에서 끝납니다.
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
새로운 기능
리팩토
✏️ Tip: You can customize this high-level summary in your review settings.