Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 56 additions & 39 deletions src/components/coffeechat/CoffeeChatCategory/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { colors } from '@sopt-makers/colors';
import { fonts } from '@sopt-makers/fonts';
import { IconChevronDown } from '@sopt-makers/icons';
import { SearchField, SelectV2 } from '@sopt-makers/ui';
import { useEffect, useState } from 'react';
import { useRouter } from 'next/router';
import { ParsedUrlQuery } from 'querystring';
import { useState } from 'react';

import { useGetMembersCoffeeChat } from '@/api/endpoint/members/getMembersCoffeeChat';
import CoffeeChatCard from '@/components/coffeechat/CoffeeChatCard';
Expand All @@ -19,6 +21,7 @@ import Loading from '@/components/common/Loading';
import Responsive from '@/components/common/Responsive';
import { LoggingClick } from '@/components/eventLogger/components/LoggingClick';
import useEventLogger from '@/components/eventLogger/hooks/useEventLogger';
import { usePageQueryParams } from '@/hooks/usePageQueryParams';
import {
MB_BIG_MEDIA_QUERY,
MB_MID_MEDIA_QUERY,
Expand All @@ -30,50 +33,64 @@ import {
} from '@/styles/mediaQuery';

export default function CoffeeChatCategory() {
const [section, setSection] = useState('');
const [topicType, setTopicType] = useState('');
const [career, setCareer] = useState('');
const [part, setPart] = useState('');
const [search, setSearch] = useState('');
const router = useRouter();
const { addQueryParamsToUrl } = usePageQueryParams({ skipNull: true });

const getQueryParamAsString = (param: string | string[] | undefined): string => {
if (Array.isArray(param)) return param[0] || ''; // 배열이면 첫 번째 값, 없으면 ''
return param ?? ''; // `undefined`이면 빈 문자열 반환
};

const section = getQueryParamAsString(router.query.section);
const topicType = getQueryParamAsString(router.query.topicType);
const career = getQueryParamAsString(router.query.career);
const part = getQueryParamAsString(router.query.part);


const [clientSearch, setClientSearch] = useState('');
const [queryParams, setQueryParams] = useState({
...(section && section !== '전체' && { section: section === '프론트엔드' ? '프론트' : section }),
...(topicType && topicType !== '전체' && { topicType }),
...(career &&
career !== '전체' && {
career: career === '인턴' ? '인턴 경험만 있어요' : career === '아직 없음' ? '아직 없어요' : career,
}),
...(part && part !== '전체' && { part }),
...(search && { search }), // search는 빈 문자열이 아닌 경우만 추가}
});
useEffect(() => {
setQueryParams({
...(section && section !== '전체' && { section: section === '프론트엔드' ? '프론트' : section }),
...(topicType && topicType !== '전체' && { topicType }),
...(career &&
career !== '전체' && {
career: career === '인턴' ? '인턴 경험만 있어요' : career === '아직 없음' ? '아직 없어요' : career,
}),
...(part && part !== '전체' && { part }),
...(search && { search }), // search는 빈 문자열이 아닌 경우만 추가}
});
}, [section, topicType, career, part, search]);

const handleSelectSection = (selected: string) => {
addQueryParamsToUrl({ section: selected !== '전체' ? selected : undefined });
};

const handleSelectTopic = (selected: string) => {
addQueryParamsToUrl({ topicType: selected !== '전체' ? selected : undefined });
};

const handleSelectCareer = (selected: string) => {
addQueryParamsToUrl({ career: selected !== '전체' ? selected : undefined });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 로직이 다른 코드에서는 아래와 같이 사용돼서 같은 목적의 사용이라면 호출부가 통일되었으면 좋겠다는 생각을 했습니다.

    addQueryParamsToUrl({ search: '' });

분명 qs의 querystring 메소드는 "" 값을 받으면 key값을 지우는게 아니라 search= 방식으로 남아있는것으로 알고있었는데 내부 로직에 lodash-es의 isEmpty가 nullable한 값들을 모두 걸러주면서 정상 작동하고 있더군요!
해당 로직을 일관성있게 처리하려면

  1. isEmpty를 사용하지 않거나, (qs.stringify는 undefined 값을 가진 key는 자동으로 serialize 대상에서 제외하기때문에 ""와 같은 사용은 문제를 일으킴)

  2. addQueryParamsToUrl의 파라미터 타입을 아래와 같이 변경하는 방법이 있을 것 같습니다.
    다만 아래의 방법은 기존의 NextRouter["query"] 타입을 사용하는것보다 직관성이나 가독성이 떨어지기 때문에 트레이드 오프가 있을 것 같군요...

export type SafeQueryParamValue = string & { __nonEmpty?: never } | null | undefined;
export type SafeQueryParams = Record<string, SafeQueryParamValue>;

제가 코드 파악하면서 스윽 둘러본 것이니 이 리뷰는 간단한 코멘트만 남겨주시고 그냥 무시하셔도 좋습니다.!

PS) NextRouter의 query 타입 뭐 대단한게 있나 보았는데 간단하네여

type ParsedUrlQuery = { [key: string]: string | string[] | undefined };
interface NextRouter {
  query: ParsedUrlQuery;
  // ... 그 외 push, pathname 등
}

};

const handleSelectPart = (selected: string) => {
addQueryParamsToUrl({ part: selected !== '전체' ? selected : undefined });
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 4개의 함수 아래처럼 로직 작성한다면 반복되는 로직 제거하고 가독성을 높일 수 있을 것 같아요!

const handleSelect = (key: string, selected: string) => {
  addQueryParamsToUrl({
    [key]: selected !== '전체' ? selected : undefined,
  });
};

const formatSoptActivities = (soptActivities: string[]) => {
const generations = soptActivities
.map((item) => parseInt(item.match(/^\d+/)?.[0] || '', 10)) // 숫자 문자열을 숫자로 변환
.filter((num) => !isNaN(num)); // NaN 값 제거
const parts = [...new Set(soptActivities.map((item) => item.replace(/^\d+기 /, '')))];
return { generation: generations, part: parts };
};
const { data, isLoading } = useGetMembersCoffeeChat(queryParams);

const formatQueryParams = (query: ParsedUrlQuery): { [key: string]: string } => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 코드에서 normalizeCareer, normalizeSection...===('전체') return undefined; 등 값을 변환하는 로직이 반복적으로 적용이 되고 있어서 해당 로직을 합쳐서 처리한다면 중복 코드가 제거되고 가독성이 올라갈 수 있을 것 같아요!

// string만 취급하도록 변환
return Object.fromEntries(
Object.entries(query)
.map(([key, value]) => [key, Array.isArray(value) ? value[0] : value])
.filter(([_, value]) => typeof value === 'string')
);
};

const { data, isLoading } = useGetMembersCoffeeChat(formatQueryParams(router.query));

const { logSubmitEvent } = useEventLogger();
const SelectionArea = (): JSX.Element => {
return (
<>
<SelectV2.Root
className='topic-select'
onChange={(e: number) => setTopicType(TOPIC_FILTER_OPTIONS[e - 1].label)}
onChange={(e: number) => handleSelectTopic(TOPIC_FILTER_OPTIONS[e - 1].label)}
type='text'
defaultValue={TOPIC_FILTER_OPTIONS.find((option) => option.label === topicType)}
visibleOptions={4}
Expand All @@ -100,7 +117,7 @@ export default function CoffeeChatCategory() {

<SelectV2.Root
className='career-select'
onChange={(e: number) => setCareer(CAREER_FILTER_OPTIONS[e - 1].label)}
onChange={(e: number) => handleSelectCareer(CAREER_FILTER_OPTIONS[e - 1].label)}
defaultValue={CAREER_FILTER_OPTIONS.find((option) => option.label === career)}
type='text'
visibleOptions={4}
Expand All @@ -127,7 +144,7 @@ export default function CoffeeChatCategory() {

<SelectV2.Root
className='part-select'
onChange={(e: number) => setPart(PART_FILTER_OPTIONS[e - 1].label)}
onChange={(e: number) => handleSelectPart(PART_FILTER_OPTIONS[e - 1].label)}
defaultValue={PART_FILTER_OPTIONS.find((option) => option.label === part)}
type='text'
visibleOptions={4}
Expand Down Expand Up @@ -165,7 +182,7 @@ export default function CoffeeChatCategory() {
<LoggingClick eventKey='coffeechatSection' key={option.categoryName} param={{ section: option.categoryName }}>
<CategoryCard
isActive={section === option.categoryName}
onClick={() => setSection(option.categoryName)}
onClick={() => handleSelectSection(option.categoryName)}
key={option.categoryName}
>
<CardIcon src={option.icon}></CardIcon>
Expand All @@ -187,7 +204,7 @@ export default function CoffeeChatCategory() {
logSubmitEvent('searchCoffeeChat', {
search_content: clientSearch,
});
setSearch(clientSearch);
addQueryParamsToUrl({ search: clientSearch || undefined });
}}
onReset={() => setClientSearch('')}
/>
Comment on lines +229 to 234

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

검색 초기화 시 search 쿼리 파라미터가 남습니다

handleFilterChange('search', '')로 빈 문자열을 전달하면 skipNull: true 옵션에도 불구하고 '' 값이 그대로 URL에 유지됩니다.
또한 로그에 실제 입력값이 아닌 기존 search 값을 사용하고 있어 분석 데이터가 부정확할 수 있습니다.

-  logSubmitEvent('searchCoffeeChat', {
-    search_content: search,
-  });
-  addQueryParamsToUrl({ search: clientSearch || undefined });
+  logSubmitEvent('searchCoffeeChat', {
+    search_content: clientSearch,
+  });
+  addQueryParamsToUrl({ search: clientSearch || undefined });
...
- onReset={() => handleFilterChange('search', '')}
+ onReset={() => handleFilterChange('search', undefined)}

이렇게 수정하면

  1. 실제 사용자가 입력한 값을 로그에 남기고
  2. 검색어를 비울 때 쿼리 파라미터가 완전히 제거됩니다.
📝 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.

Suggested change
search_content: search,
});
setSearch(clientSearch);
addQueryParamsToUrl({ search: clientSearch || undefined });
}}
onReset={() => setClientSearch('')}
onReset={() => handleFilterChange('search', '')}
/>
logSubmitEvent('searchCoffeeChat', {
search_content: clientSearch,
});
addQueryParamsToUrl({ search: clientSearch || undefined });
}}
onReset={() => handleFilterChange('search', undefined)}
/>
🤖 Prompt for AI Agents
In src/components/coffeechat/CoffeeChatCategory/index.tsx around lines 229 to
234, the onReset handler calls handleFilterChange with an empty string, but this
leaves the 'search' query parameter in the URL and logs the old search value
instead of the actual input. To fix this, update the onReset function to pass
undefined or null instead of an empty string to handleFilterChange so that the
'search' parameter is removed from the URL, and ensure the logging uses the
current input value to accurately reflect user actions.

Expand All @@ -202,15 +219,15 @@ export default function CoffeeChatCategory() {
logSubmitEvent('searchCoffeeChat', {
search_content: clientSearch,
});
setSearch(clientSearch);
addQueryParamsToUrl({ search: clientSearch || undefined });
}}
onReset={() => setClientSearch('')}
/>
<StyledMobileFilterWrapper>
<StyledMobileFilter
value={section}
onChange={(e: string) => {
setSection(SECTION_FILTER_OPTIONS[parseInt(e) - 1].label);
handleSelectSection(SECTION_FILTER_OPTIONS[parseInt(e) - 1].label);
}}
options={SECTION_FILTER_OPTIONS.map((option) => ({
value: option.value.toString(),
Expand All @@ -234,7 +251,7 @@ export default function CoffeeChatCategory() {
>
<StyledMobileFilter
value={topicType}
onChange={(e: string) => setTopicType(TOPIC_FILTER_OPTIONS[parseInt(e) - 1].label)}
onChange={(e: string) => handleSelectTopic(TOPIC_FILTER_OPTIONS[parseInt(e) - 1].label)}
options={TOPIC_FILTER_OPTIONS.map((option) => ({
value: option.value.toString(),
label: option.label,
Expand All @@ -258,7 +275,7 @@ export default function CoffeeChatCategory() {
>
<StyledMobileFilter
value={career}
onChange={(e: string) => setCareer(CAREER_FILTER_OPTIONS[parseInt(e) - 1].label)}
onChange={(e: string) => handleSelectCareer(CAREER_FILTER_OPTIONS[parseInt(e) - 1].label)}
options={CAREER_FILTER_OPTIONS.map((option) => ({
value: option.value.toString(),
label: option.label,
Expand All @@ -282,7 +299,7 @@ export default function CoffeeChatCategory() {
>
<StyledMobileFilter
value={part}
onChange={(e: string) => setPart(PART_FILTER_OPTIONS[parseInt(e) - 1].label)}
onChange={(e: string) => handleSelectPart(PART_FILTER_OPTIONS[parseInt(e) - 1].label)}
options={PART_FILTER_OPTIONS.map((option) => ({
value: option.value.toString(),
label: option.label,
Expand Down