Conversation
![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
This pull request migrates organization state management from URL query parameters to a persistent Zustand store, removing the useOrganizationId hook and updating several components to use the new useOrganizations store. Feedback includes addressing a hardcoded fallback value in the school selection logic by restoring automatic selection of the first available organization, removing a redundant null check in the year filter, and defining the default organization ID as a constant for improved maintainability.
| import { Suspense } from 'react'; | ||
|
|
||
| import { useSuspenseOrganizations } from '~/api/queries/useOrganizations'; | ||
| import { Select } from '~/components/ui/select'; | ||
| import { useOrganizationId } from '~/hooks/useOrganizationId'; | ||
| import { useOrganizations } from '~/stores/use-organization'; | ||
|
|
||
| const SchoolSelectContent = () => { | ||
| const { data: organizations } = useSuspenseOrganizations(); | ||
| const { organizationId, setOrganizationId } = useOrganizationId(); | ||
| const { organizationId, setOrganization } = useOrganizations(); | ||
|
|
||
| const options = organizations.map((org) => ({ value: org.id, label: org.name })); | ||
| const isValidId = options.some((opt) => opt.value === organizationId); | ||
| const selectedId = isValidId ? organizationId : organizations[0]?.id; | ||
| const selectedId = isValidId ? organizationId : undefined; | ||
|
|
||
| useEffect(() => { | ||
| if (!isValidId && selectedId !== undefined) { | ||
| startTransition(() => { | ||
| setOrganizationId(selectedId, { scroll: false, history: 'replace' }); | ||
| }); | ||
| } | ||
| }, [isValidId, selectedId, setOrganizationId]); | ||
|
|
||
| const handleChange = (id: number) => { | ||
| startTransition(() => { | ||
| setOrganizationId(id, { scroll: false, history: 'replace' }); | ||
| }); | ||
| }; | ||
|
|
||
| if (selectedId === undefined) return null; | ||
|
|
||
| return <Select value={selectedId} onChange={handleChange} options={options} />; | ||
| return ( | ||
| <Select value={selectedId ?? 2} onChange={(id) => setOrganization(id)} options={options} /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
현재 구현에서는 organizationId가 유효하지 않을 경우 하드코딩된 값 2를 사용하고 있습니다. 이는 데이터 변경에 취약하며 예기치 않은 동작을 유발할 수 있습니다. 이전 구현처럼 유효하지 않은 ID를 감지하고, 목록의 첫 번째 유효한 ID로 전역 상태를 업데이트하는 것이 더 안정적인 방법입니다. 이를 위해 useEffect를 다시 import해야 합니다.
import { Suspense, useEffect } from 'react';
import { useSuspenseOrganizations } from '~/api/queries/useOrganizations';
import { Select } from '~/components/ui/select';
import { useOrganizations } from '~/stores/use-organization';
const SchoolSelectContent = () => {
const { data: organizations } = useSuspenseOrganizations();
const { organizationId, setOrganization } = useOrganizations();
const options = organizations.map((org) => ({ value: org.id, label: org.name }));
const isValidId = options.some((opt) => opt.value === organizationId);
useEffect(() => {
const firstId = organizations[0]?.id;
if (!isValidId && firstId !== undefined) {
setOrganization(firstId);
}
}, [isValidId, organizations, setOrganization]);
const selectedId = isValidId ? organizationId : organizations[0]?.id;
if (selectedId === undefined) {
return null;
}
return <Select value={selectedId} onChange={setOrganization} options={options} />;
};
| year: _year, | ||
| ...(sport && { sport }), | ||
| ...(organizationId && { organizationId }), | ||
| ...(organizationId !== null && { organizationId }), |
There was a problem hiding this comment.
useOrganizations 스토어에서 가져온 organizationId는 number 타입이며 null이 될 수 없습니다. 따라서 organizationId !== null 확인은 불필요하며 코드를 단순화할 수 있습니다.
| ...(organizationId !== null && { organizationId }), | |
| organizationId, |
| export const useOrganizations = create<OrganizationState>()( | ||
| persist( | ||
| (set) => ({ | ||
| organizationId: 2, |
There was a problem hiding this comment.
초기 organizationId로 하드코딩된 값 2를 사용하고 있습니다. 가독성과 유지보수성을 높이기 위해 파일 상단에 const DEFAULT_ORGANIZATION_ID = 2;와 같이 상수로 정의하고, 그 상수를 여기서 사용하는 것이 좋습니다.
| organizationId: 2, | |
| organizationId: 2, // TODO: Consider defining this as a constant, e.g., DEFAULT_ORGANIZATION_ID |
sungwonnoh
left a comment
sungwonnoh
left a comment
There was a problem hiding this comment.
전역 상태로 관리하는 건 좋은 것 같아요! 지금은 경희대 대회기간이라 organization id를 2로 고정해둬서 괜찮은데, 만약 외대 기간에는 값을 계속 바꿔줘야하는 걸로 보이네요.
초반 아이디어는 인스타처럼 url공유할 때 학교 별로 구분하려고 쿼리 스트링에 organization id를 넣었어요 혹시 이부분에 대해서 어떻게 생각하셨나용 ??
|
이거 쿼리스트링도 문제가 많은 거 같아서 슬랙에서 한 번 논의해볼게요 |
2 participants
✅ 작업 내용
zustand설치📝 참고 자료
♾️ 기타