[Refactor] pagination 상태 유지 개선(searchParams)#158
Conversation
- useSearchParams 적용하여 뒤로가기 시에도 검색어와 페이지 유지되도록 수정 - useEffect 통해 검색어 유지되도록 수정
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExperience 타입을 유니온에서 문자열로 완화하고 관련 API/컴포넌트 타입을 문자열 기반으로 업데이트했으며, 여러 페이지 컴포넌트의 필터·페이징 상태 관리를 로컬 state에서 React Router의 URL 검색 파라미터(useSearchParams)로 마이그레이션했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant 사용자 as 사용자
participant 페이지 as 페이지 컴포넌트
participant 라우터 as React Router (useSearchParams)
participant 훅 as API 훅 (useGetExperienceList / useGetAiReportList)
participant 서버 as 서버/API
사용자->>페이지: 방문 또는 필터/페이징/검색 조작
페이지->>라우터: searchParams 읽기/갱신 (type, page, keyword ...)
라우터-->>페이지: 현재 쿼리파라미터 반환
페이지->>훅: 데이터 요청 (type: string, page: number, ...)
훅->>서버: HTTP 요청 (쿼리 파라미터 포함)
서버-->>훅: 응답 데이터
훅-->>페이지: 데이터 반환
페이지->>사용자: UI 렌더링 (목록, 페이징, 필터 상태)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
🚀 빌드 결과✅ 린트 검사 완료 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/experience/api/use-experience-list.query.ts (1)
29-32:⚠️ Potential issue | 🟠 Major
queryKey와queryFn에서 falsy 값 처리가 불일치해요.queryKey: experienceQueryKey.list(type ?? "", page), // "" 으로 정규화 queryFn: () => getExperienceList({ type: type ?? undefined, page }), // undefined로 정규화이로 인해 캐시 동작에 문제가 발생할 수 있어요:
type=null,type="",type=undefined가 모두 같은 캐시 키("")를 공유하지만- 실제 API 호출은 다르게 동작해요 (
""vsundefined쿼리 파라미터)캐시 키와 API 호출이 일관되게 동작하도록 통일해주세요.
🔧 일관된 처리를 위한 제안
export const useGetExperienceList = ({ type, page, }: { type?: string | null; page: number; }) => { + const normalizedType = type || undefined; return useQuery({ - queryKey: experienceQueryKey.list(type ?? "", page), - queryFn: () => getExperienceList({ type: type ?? undefined, page }), + queryKey: experienceQueryKey.list(normalizedType ?? "", page), + queryFn: () => getExperienceList({ type: normalizedType, page }), }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/experience/api/use-experience-list.query.ts` around lines 29 - 32, Normalize the `type` value the same way in both the cache key and the API call: choose a single canonical form (e.g., undefined) and apply it to both `experienceQueryKey.list(...)` and the call to `getExperienceList(...)` so the cache key matches the actual request parameters; specifically, update the `queryKey` invocation (experienceQueryKey.list) to accept the same normalized `type` as the `queryFn` that calls `getExperienceList` (both using type ?? undefined or both using type ?? ""), ensuring consistency between the cache key and the API request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/experience/experience-page.tsx`:
- Around line 34-39: handlePageChange currently uses an unnecessary nullish
coalescing (type ?? "") even though type is already derived from
searchParams.get("type") || "" and thus always a string; update the
handlePageChange function to pass type directly to setSearchParams (remove the
"?? \"\"" usage) so setSearchParams({ type: type, page: String(page) }) uses the
existing type variable from the searchParams.get("type") || "" initialization.
In `@src/widgets/experience-filter/experience-filter.tsx`:
- Line 22: Guard the cast of value to ExperienceTypeCode before indexing
EXPERIENCE_TYPE: replace the unvalidated cast in the experience-filter component
by checking membership first (e.g., if (value &&
Object.prototype.hasOwnProperty.call(EXPERIENCE_TYPE, value)) triggerLabel =
EXPERIENCE_TYPE[value as ExperienceTypeCode];) so that triggerLabel cannot
become undefined when an invalid string is passed; reference the symbols value,
EXPERIENCE_TYPE, ExperienceTypeCode, and triggerLabel in your change.
---
Outside diff comments:
In `@src/features/experience/api/use-experience-list.query.ts`:
- Around line 29-32: Normalize the `type` value the same way in both the cache
key and the API call: choose a single canonical form (e.g., undefined) and apply
it to both `experienceQueryKey.list(...)` and the call to
`getExperienceList(...)` so the cache key matches the actual request parameters;
specifically, update the `queryKey` invocation (experienceQueryKey.list) to
accept the same normalized `type` as the `queryFn` that calls
`getExperienceList` (both using type ?? undefined or both using type ?? ""),
ensuring consistency between the cache key and the API request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cdcf287-49f4-4da2-a5be-0ec5d6d95d1c
📒 Files selected for processing (8)
src/features/experience-detail/types/experience-detail.types.tssrc/features/experience-detail/ui/experience-viewer/experience-viewer.tsxsrc/features/experience/api/use-experience-list.query.tssrc/pages/experience/experience-page.tsxsrc/pages/home/search-section/search-section.tsxsrc/pages/matching-list/matching-list-page.tsxsrc/shared/api/generate/http-client.tssrc/widgets/experience-filter/experience-filter.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/experience/experience-page.tsx`:
- Line 17: The code forces searchParams.get("type") to an empty string which
breaks the "no-filter selected" state; revert to letting const type =
searchParams.get("type") return null when absent and update any places that read
or set this param (the var name type and uses of searchParams.get) to treat null
as "unset" — when updating the URL/query (e.g., via your setSearchParams or
related hooks referenced around lines 22-23, 28-31, 35-37) remove the key
entirely instead of writing type=""; ensure any consumer hooks accept
null/undefined as the unset state rather than an empty string.
- Line 16: The page query isn't being normalized so values like "-1" or "1.5"
slip through; read the raw value from searchParams.get("page"), parse it with
parseInt(raw, 10) (or Math.floor(Number(raw))), validate that the parsed value
is an integer >= 1, and if it's NaN or < 1 fall back to 1; replace the current
currentPage assignment (which uses Number(searchParams.get("page")) || 1) with
this defensive parsing/validation logic so currentPage is always a positive
integer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31c762cb-a7b0-44b0-b011-746c1ef767a1
📒 Files selected for processing (1)
src/pages/experience/experience-page.tsx
odukong
left a comment
odukong
left a comment
There was a problem hiding this comment.
추추의 QA .. 완벽하게 완료해주셨네요..!
특히 searchParams 업데이트 로직을 아래처럼 updateSearchParams로 추상화해서 url 관련 상태들을 업데이트 할 때 하나의 함수로 재사용이 가능해져서 전체 코드 흐름도 볼 때도 깔끔해지게 된 것 같아요🥸💕
Object.entries(patch).forEach(([key, value]) => {
if (value === undefined || value === "") {
newParams.delete(key);
} else {
newParams.set(key, String(value));
}
});
수고하셨뜹니다 ~~~ o(^＠^)o
|
상태를 URL 쿼리 파라미터로 올려서 상세 페이지에서 뒤로 돌아왔을 때도 기존 목록 상태가 유지되도록 정리해주신 방향 너무 좋네요! 제가 작업한 파일 내에서 몇 군데 코드 상으로 조금 더 명확하게 정리되면 좋을 것 같은 부분만 아래 코멘트로 남겨두겠습니다. 수고 많으셨습니다! 👍 |
| if (!patch.page) { | ||
| newParams.set("page", "1"); | ||
| } |
There was a problem hiding this comment.
패치된 값 기준으로 page를 자동으로 1로 돌려주는 방식 좋은 것 같습니다 👍
다만 현재는 !patch.page 조건이라 page: 0 같은 값도 동일하게 reset 대상으로 들어가게 되어 의미가 조금 모호해질 수 있다고 생각하는데, “이번 patch에 page가 포함되지 않은 경우”라는 의도를 더 명확히 드러내기 위해 !("page" in patch)처럼 표현하는 방식은 어떨까요? 🙂
There was a problem hiding this comment.
좋은 지적 감사합니다! 유진님 말씀대로 조건문의 목적을 고려했을 때 제안해주신 방향으로 수정하는 게 더 적합한 것 같습니다! 반영하겠습니다 😀
qowjdals23
left a comment
qowjdals23
left a comment
There was a problem hiding this comment.
페이지네이션 상태 유지 !!! 뒤로가기 했을 때도 목록 상태가 유지되도록 searchParams 기준으로 잘 정리해주신 것 같고, 흐름도 한눈에 보기 더 좋아진 것 같네요 !!! 👏
여러 페이지 리팩토링하시느라 고민도 많으셨을텐데 너무너무 수고많으셨습니다 ~~ 많이 배워가용 🔥
| const currentPage = Number(searchParams.get("page")) || 1; | ||
| const type = searchParams.get("type") || ""; | ||
|
|
||
| const [isExpTouched, setIsExpTouched] = useState(false); |
There was a problem hiding this comment.
사소한 부분인데요..!
필터 값은 URL 에서 잘 복원되지만 isExpTouched는 로컬 state라 뒤로가기하면 다시 false로 초기화 될 것 같은데 혹시 의도한 동작인건지 궁금해서 리뷰 남깁니다 ! 👀
There was a problem hiding this comment.
말씀주신 대로 현재는 isExpTouched가 state이기 때문에, 페이지 리렌더링 시 false로 초기화 되고 있습니다. 하지만 현재 로직상 type 파라미터가 비어있을 때 '전체' 데이터를 보여주고 있어서 필터 라벨이 기본값(경험 유형)으로 돌아가더라도 UX 측면에서 큰 지장이 없다고 생각해 지금과 같은 방식으로 구현했습니다 😀
허나 ‘전체’와 ‘경험 유형’은 엄연히 다른 상태이기 때문에 구분하는 것이 더 좋을 것이라고 생각해요! 우선 우선순위가 더 높은 쿼리스트링값의 유지를 우선적으로 구현하고 해당 부분은 추후 스프린트에 적용할까 하는데 정민님 생각은 어떠신가요??
There was a problem hiding this comment.
넵 이해했습니다!
말씀해주신 것처럼 현재 PR에서는 쿼리스트링 기반으로 목록 상태를 유지하는 쪽이 우선순위가 더 높아 보여서 ! 지금 방향으로 가도 괜찮을 것 같아요 👍
전체 / 경험 유형 구분 부분은 추후 스프린트에서 보완되면 더 좋을 것 같습니다!! 설명 감사합니다 😄
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/experience/experience-page.tsx`:
- Around line 107-111: The loading message "기업 정보를 불러오고 있어요" is wrong for the
Experience page; update the JSX inside the isLoading branch (the <img
src={CAT_SPINNER} ... /> and <p className={styles.spinnerText}>...</p> block) to
use a page-appropriate message such as "경험 목록을 불러오고 있어요" (or "경험 정보를 불러오고 있어요")
by replacing the string in the <p className={styles.spinnerText}> element; keep
the rest (isLoading check, CAT_SPINNER, styles.spinner and styles.spinnerText)
unchanged.
- Around line 52-73: The page-correction useEffect (useEffect block referencing
currentPage, totalPage, isInvalidNumber, isValidType, typeParam, searchParams,
setSearchParams) runs before data loads because totalPage defaults to 1; update
the effect to only perform validation when data loading is complete (e.g., check
an isLoading or isDataLoaded flag) so the isExceeding check uses a real
totalPage and avoid resetting page prematurely; ensure the dependency array
includes that loading flag and the conditional early-returns if loading.
- Around line 22-25: The current page validation (variables isInvalidNumber and
currentPage using pageParam) allows fractional values like "1.5"; update the
isInvalidNumber expression to also reject non-integer numbers by adding a
Number.isInteger(Number(pageParam)) check (e.g., mark invalid when pageParam !==
null && (!Number.isInteger(Number(pageParam)) || isNaN(Number(pageParam)) ||
Number(pageParam) < 1)), then compute currentPage the same way so fractional
pageParam values resolve to 1.
- Around line 27-28: The URL retains an empty `type` query because you set
`type` to "" but the useEffect only removes `type` when `typeParam` exists and
is invalid; update the useEffect condition to also delete the query when the
resolved `type` is an empty string so `newParams.delete("type")` runs for both
invalid params and cleared filters—locate the useEffect that references
`typeParam`, `isValidType`, and `type` and change the conditional to include
`type === ""` (e.g., if ((typeParam && !isValidType) || type === "") then call
newParams.delete("type")).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c8109fc-89a4-44f5-a439-1ceeca9a00b5
📒 Files selected for processing (2)
src/pages/experience/experience-page.css.tssrc/pages/experience/experience-page.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/home/search-section/search-section.tsx (1)
167-190: 🧹 Nitpick | 🔵 Trivial필터/토글 이벤트 핸들러를
handle*네이밍의 명시 함수로 분리해 주세요현재 Line [167], Line [176], Line [188]의 인라인 핸들러는 추적/재사용/테스트 측면에서 일관성이 떨어집니다.
handleIndustryChange,handleScaleChange,handleRecruitedChange로 분리하면 의도가 더 명확해집니다.As per coding guidelines, "Use handle prefix for event handler functions and on prefix for event handler props (e.g.,
handleButtonClick,onAlertClick)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/home/search-section/search-section.tsx` around lines 167 - 190, Extract the inline handlers into named functions: create handleIndustryChange(newIndustry) that calls setIsIndustryTouched(true) and updateSearchParams({ industry: newIndustry }); create handleScaleChange(newScale) that calls setIsScaleTouched(true) and updateSearchParams({ scale: newScale }); and create handleRecruitedChange(checked) that calls updateSearchParams({ isRecruited: checked }); then replace the inline onChange/onCheckedChange props on the Industry filter, ScaleFilter, and Toggle with these handle* functions (referencing setIsIndustryTouched, setIsScaleTouched, updateSearchParams and isRecruited to locate state/props). Ensure the handlers use the same parameter names used by the components and are defined in the same component scope so they can be tested and reused.
♻️ Duplicate comments (1)
src/pages/home/search-section/search-section.tsx (1)
30-43:⚠️ Potential issue | 🟠 Major필터 코드 검증 로직이 실제 코드셋과 불일치하고,
null단언으로 잘못된 파라미터가 전파됩니다Line [32] / Line [41]의 하드코딩 목록이 실제
IndustryCode/ScaleCode정의와 달라서 유효한 URL 값이 삭제될 수 있습니다. 또한 Line [31], Line [36], Line [42] 흐름에서는null이as IndustryCode/as ScaleCode로 통과되어 런타임에서params로 전달될 수 있습니다. 단언 대신 “실제 코드셋 기반 파싱 +undefined반환”으로 바꾸는 게 안전합니다.수정 예시
+import { FILTER_INDUSTRY, FILTER_SCALE } from "@/shared/config/company"; import type { IndustryCode, ScaleCode } from "@/shared/config"; @@ - const isValidIndustry = - industryParam === null || - ["IT_SERVICE", "COMMERCE", "FINANCE", "CONSUMER_GOODS"].includes( - industryParam - ); - const industry = isValidIndustry - ? (industryParam as IndustryCode) - : undefined; + const INDUSTRY_CODES = new Set( + FILTER_INDUSTRY.map(({ code }) => code) + ); + const industry = + industryParam && INDUSTRY_CODES.has(industryParam as IndustryCode) + ? (industryParam as IndustryCode) + : undefined; @@ - const isValidScale = - scaleParam === null || - ["STARTUP", "SMALL", "MID_LARGE", "LARGE"].includes(scaleParam); - const scale = isValidScale ? (scaleParam as ScaleCode) : undefined; + const SCALE_CODES = new Set(FILTER_SCALE.map(({ code }) => code)); + const scale = + scaleParam && SCALE_CODES.has(scaleParam as ScaleCode) + ? (scaleParam as ScaleCode) + : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/home/search-section/search-section.tsx` around lines 30 - 43, The hardcoded validation lists in isValidIndustry/isValidScale must be replaced by checking against the real code sets (use the canonical IndustryCode and ScaleCode enums/arrays) and remove the null/assert casts; change the logic so industryParam and scaleParam are parsed against those canonical sets and if not matched return undefined (do not cast null to IndustryCode/ScaleCode). Update the variables industry and scale to be derived from the safe-parse result (e.g., parseIndustry(industryParam) and parseScale(scaleParam)) so invalid or null inputs yield undefined rather than being asserted through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/home/search-section/search-section.tsx`:
- Around line 63-99: The effect that enforces URL normalization can run too
early because placeholderData keeps a stale totalPage; update the guard in the
useEffect that computes isExceedingPage so it also returns when placeholder data
is present—i.e., add isPlaceholderData alongside isLoading in the early
return—and leave the rest of the logic (isExceedingPage, isInvalidRecruited,
constructing newParams, and calling setSearchParams with replace) unchanged;
this ensures currentPage/totalPage corrections only run on real data rather than
while previousData is being used.
---
Outside diff comments:
In `@src/pages/home/search-section/search-section.tsx`:
- Around line 167-190: Extract the inline handlers into named functions: create
handleIndustryChange(newIndustry) that calls setIsIndustryTouched(true) and
updateSearchParams({ industry: newIndustry }); create
handleScaleChange(newScale) that calls setIsScaleTouched(true) and
updateSearchParams({ scale: newScale }); and create
handleRecruitedChange(checked) that calls updateSearchParams({ isRecruited:
checked }); then replace the inline onChange/onCheckedChange props on the
Industry filter, ScaleFilter, and Toggle with these handle* functions
(referencing setIsIndustryTouched, setIsScaleTouched, updateSearchParams and
isRecruited to locate state/props). Ensure the handlers use the same parameter
names used by the components and are defined in the same component scope so they
can be tested and reused.
---
Duplicate comments:
In `@src/pages/home/search-section/search-section.tsx`:
- Around line 30-43: The hardcoded validation lists in
isValidIndustry/isValidScale must be replaced by checking against the real code
sets (use the canonical IndustryCode and ScaleCode enums/arrays) and remove the
null/assert casts; change the logic so industryParam and scaleParam are parsed
against those canonical sets and if not matched return undefined (do not cast
null to IndustryCode/ScaleCode). Update the variables industry and scale to be
derived from the safe-parse result (e.g., parseIndustry(industryParam) and
parseScale(scaleParam)) so invalid or null inputs yield undefined rather than
being asserted through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31e8e2d2-2af7-4851-b915-70b9c9bd33f4
📒 Files selected for processing (1)
src/pages/home/search-section/search-section.tsx
| // URL 강제 교정 | ||
| useEffect(() => { | ||
| if (isLoading) return; | ||
|
|
||
| const isExceedingPage = currentPage > totalPage && totalPage > 0; | ||
| const isInvalidRecruited = | ||
| recruitedParam !== null && | ||
| recruitedParam !== "true" && | ||
| recruitedParam !== "false"; | ||
|
|
||
| if ( | ||
| isInvalidPage || | ||
| isExceedingPage || | ||
| !isValidIndustry || | ||
| !isValidScale || | ||
| isInvalidRecruited | ||
| ) { | ||
| const newParams = new URLSearchParams(searchParams); | ||
|
|
||
| if (isInvalidPage || isExceedingPage) newParams.set("page", "1"); | ||
| if (!isValidIndustry) newParams.delete("industry"); | ||
| if (!isValidScale) newParams.delete("scale"); | ||
| if (isInvalidRecruited) newParams.set("isRecruited", "true"); | ||
|
|
||
| setSearchParams(newParams, { replace: true }); | ||
| } | ||
| }, [ | ||
| currentPage, | ||
| totalPage, | ||
| isInvalidPage, | ||
| isValidIndustry, | ||
| isValidScale, | ||
| recruitedParam, | ||
| isLoading, | ||
| searchParams, | ||
| setSearchParams, | ||
| ]); |
There was a problem hiding this comment.
placeholderData 상태에서 페이지 강제 교정이 조기 실행될 수 있습니다
useGetCompanies가 placeholderData: previousData를 사용하므로, 새 쿼리 전환 직후에는 이전 totalPage가 잠시 유지됩니다. 현재 Line [65]는 isLoading만 가드해서, placeholder 구간에 Line [67]의 isExceedingPage가 참이 되면 정상 페이지도 1로 리셋될 수 있습니다. isPlaceholderData도 함께 가드해 주세요.
수정 예시
- useEffect(() => {
- if (isLoading) return;
+ useEffect(() => {
+ if (isLoading || isPlaceholderData) return;
@@
}, [
@@
isLoading,
+ isPlaceholderData,
searchParams,
setSearchParams,
]);📝 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.
| // URL 강제 교정 | |
| useEffect(() => { | |
| if (isLoading) return; | |
| const isExceedingPage = currentPage > totalPage && totalPage > 0; | |
| const isInvalidRecruited = | |
| recruitedParam !== null && | |
| recruitedParam !== "true" && | |
| recruitedParam !== "false"; | |
| if ( | |
| isInvalidPage || | |
| isExceedingPage || | |
| !isValidIndustry || | |
| !isValidScale || | |
| isInvalidRecruited | |
| ) { | |
| const newParams = new URLSearchParams(searchParams); | |
| if (isInvalidPage || isExceedingPage) newParams.set("page", "1"); | |
| if (!isValidIndustry) newParams.delete("industry"); | |
| if (!isValidScale) newParams.delete("scale"); | |
| if (isInvalidRecruited) newParams.set("isRecruited", "true"); | |
| setSearchParams(newParams, { replace: true }); | |
| } | |
| }, [ | |
| currentPage, | |
| totalPage, | |
| isInvalidPage, | |
| isValidIndustry, | |
| isValidScale, | |
| recruitedParam, | |
| isLoading, | |
| searchParams, | |
| setSearchParams, | |
| ]); | |
| // URL 강제 교정 | |
| useEffect(() => { | |
| if (isLoading || isPlaceholderData) return; | |
| const isExceedingPage = currentPage > totalPage && totalPage > 0; | |
| const isInvalidRecruited = | |
| recruitedParam !== null && | |
| recruitedParam !== "true" && | |
| recruitedParam !== "false"; | |
| if ( | |
| isInvalidPage || | |
| isExceedingPage || | |
| !isValidIndustry || | |
| !isValidScale || | |
| isInvalidRecruited | |
| ) { | |
| const newParams = new URLSearchParams(searchParams); | |
| if (isInvalidPage || isExceedingPage) newParams.set("page", "1"); | |
| if (!isValidIndustry) newParams.delete("industry"); | |
| if (!isValidScale) newParams.delete("scale"); | |
| if (isInvalidRecruited) newParams.set("isRecruited", "true"); | |
| setSearchParams(newParams, { replace: true }); | |
| } | |
| }, [ | |
| currentPage, | |
| totalPage, | |
| isInvalidPage, | |
| isValidIndustry, | |
| isValidScale, | |
| recruitedParam, | |
| isLoading, | |
| isPlaceholderData, | |
| searchParams, | |
| setSearchParams, | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/home/search-section/search-section.tsx` around lines 63 - 99, The
effect that enforces URL normalization can run too early because placeholderData
keeps a stale totalPage; update the guard in the useEffect that computes
isExceedingPage so it also returns when placeholder data is present—i.e., add
isPlaceholderData alongside isLoading in the early return—and leave the rest of
the logic (isExceedingPage, isInvalidRecruited, constructing newParams, and
calling setSearchParams with replace) unchanged; this ensures
currentPage/totalPage corrections only run on real data rather than while
previousData is being used.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/matching-list/matching-list-page.tsx`:
- Around line 56-67: handleSearch and handlePageChange overwrite the entire URL
search params and drop any existing filters/sort; change both handlers to merge
updates into the current search params instead of replacing them: read the
current params (via the searchParams object or the setter callback), copy them,
update only keyword or page (use the existing keyword variable for page updates
and newKeyword for search), then call setSearchParams with the merged params so
other query keys are preserved; update references in handleSearch,
handlePageChange, and any call sites using setSearchParams.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f027d835-b173-4ead-bf2f-13f2d7838932
📒 Files selected for processing (1)
src/pages/matching-list/matching-list-page.tsx
|
@u-zzn 유진님의 꼼꼼한 코드리뷰 감사합니다! 사실 해당 부분 관련해서는 다음 스프린트에서 리팩토링을 고려했어요. 현재 queryString을 이용해 페이지를 파싱하는 방식은 아래와 같은 문제점이 있어요.
그리하여 toss의 queryString 라이브러리 공식 문서를 참고하여 쿼리파라미터값을 제어할 수 있는 훅을 따로 분리하는 작업을 고민중에 있습니다. 다만, 1차 스프린트 내에 진행해야 하는, 우선순위가 더 높은 작업이 존재하기 때문에 이번 스프린트에서는 다음과 같이 개선한 후, 추후에 리팩토링을 진행하고자 합니다. ⚡️ 존재하지 않는, 유효하지 않은 페이지 혹은 타입으로 접근 시 유효한 값으로 강제 교정
위와 같은 방식으로 개선 후, 다음 스프린트에서 관심사 분리를 진행하는 방식으로 개선해나가려고 하는데 여러분 생각은 어떠신가요?? 😊😊 |
|
상세하게 공유해주셔서 감사합니다! 😊 말씀해주신 것처럼 현재 구조에서는 쿼리 파라미터 파싱과 UI 로직이 일부 섞여 있고, 페이지네이션 로직도 여러 페이지에 분산되어 있어 이후 유지보수나 확장 시 부담이 될 수 있는 부분이라고 생각이 드네요. 이번 스프린트에서는 말씀해주신 방향대로 유효하지 않은 값에 대해 안전하게 보정하는 것에 집중하고, 다음 단계에서 queryString 기반으로 파싱/검증 로직을 분리하면서 관심사 분리와 중복 제거까지 함께 가져가는 흐름이 자연스럽고 좋은 접근인 것 같습니다 👍 특히 페이지 값이나 필터값을 다루는 부분은 공통 유틸이나 레이어로 분리된다면 각 페이지에서는 “사용”에만 집중할 수 있어서 구조적으로도 더 안정적일 것 같네요 🙂 이후 리팩토링 과정에서 해당 부분들이 어떻게 정리될지 기대됩니다! 💪 |
|
사용자가 올바른 쿼리 URL을 입력하지 않았을 상황을 고려하여 클라 단에서 쿼리를 강제시킴으로서 사용자는 일관된 페이지네이션 결과를 볼 수 있게 하는 것이 적절한 방향이라고 생각하고, 말씀해주신 대로 홈, 경험리스트, 매칭리스트 페이지에서 중복 코드가 작성되고 있는 상황이기 때문에 해당 이슈 역시 생각해주신 훅 분리로 리팩토링 진행해주시면 좋을 것 같아요. |
|
자세한 설명 남겨주신 덕분에 이번에 어떤 기준으로 우선순위를 잡으셨는지 잘 이해됐습니다 ! 우선 이번 스프린트에서는 유효하지 않은 쿼리값을 안전하게 교정해서 사용자 경험을 우선 보완하고, 말씀주신 것처럼 공통으로 들어가는 로직들은 다음 스프린트에서 한 번 정리되면 더 좋아질 것 같아요 !! 추후에 훅으로 분리되면 구조도 더 깔끔해질 것 같아서 기대됩니다 🤩 |
4 participants
✏️ Summary
📑 Tasks
페이지네이션의 상태가 유지되도록 리팩토링을 진행했습니다. 상세 페이지에서 '뒤로가기' 버튼을 통해 목록 페이지로 넘어왔을 때 이전 상태를 유지하려면 url에 목록에 관련된 정보들을 저장해야 하는데요, 이 때 페이지와 필터, 검색어 등 상태가 유지되어야 하는 정보들을 쿼리 파라미터로 넘겨 페이지를 옮겨다니더라도 상태가 유지되도록 구현해주었습니다.
구현 방식은 위와 같습니다. 페이지 렌더링 시
searchParams를 통해 쿼리 파라미터의 값을 가져오고, 존재하지 않을 경우 디폴트 값으로 설정되도록 수정해주었습니다. 추가적으로 인라인 함수로 구현되어 있던 부분도 컨벤션에 맞게 'handle-' 형태의 함수로 분리해주었습니다.현재 페이지네이션이 사용되는 페이지는 홈, 경험 목록, 매칭 경험 목록 이렇게 3가지인데요, 이중 홈과 매칭 경험 목록의 경우 검색 기능이 포함되어 있는데 검색값이 날아가는 것을 방지하기 위해 useEffect를 통해 검색값이 유지되도록 해주었습니다~
👀 To Reviewer
리팩토링 과정에서 ExperienceFilter 컴포넌트의 value의 type을
string | null로 확장해주었습니다. 그 이유는 .. 제가 안정성을 위해 범위를 너무 좁혀놨더니 오히려 결합도가 지나치게 올라가서 하나 수정하려고 130913개의 파일을 건너야 하는 불상사가 발생하여서 리팩토링 하는 김에 같이 바꿔주었습니다!이 부분 수정하면서 http-client의 getRequestDto와, ExperienceRequestDto의
type과 getSummaryExperienceList 메소드의type의 타입을 string으로 수정해주었습니다. 또한 유진이가 작업한 파일에서이렇게 수정되었습니다! 타입이랑, label의 기본값 정도만 수정되어서 큰 문제는 없겠지만 그래도 알아두어야 할 거 같아 전달합니다 ~
📸 Screenshot
추추의 작업 이슈로 화면녹화를 못하네요 쩝 .. 테스트 직접 해보기 ~~~~ >_<
🔔 ETC
Summary by CodeRabbit
버그 수정
리팩토링
새 기능
변경