[FIX] clubId 가 string이지 못하게 교정 (#158)#159
Conversation
Summary of ChangesHello @ganimjeong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 클럽 상세 페이지로 이동할 때 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConverts route Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router as Router
participant Page as ClubDetail Page
participant API as fetchClubDetail
participant Repo as mock repo
User->>Router: Navigate to /clubs/:clubId (string)
Router->>Page: Provide param `clubId` (string)
Note over Page: Convert to number -> `clubIdNumber`
Page->>Page: Guard on `clubIdNumber`
Page->>API: fetchClubDetail(clubIdNumber)
API->>Repo: lookup by `clubId` (mock)
Repo-->>API: ClubDetail { clubId, ... }
API-->>Page: Club data
Page-->>User: Render details + Apply button (uses club.clubId)
User->>Page: Click "Apply"
Page->>Router: Navigate to /apply/:clubId (uses numeric clubId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
이번 PR은 clubId가 string으로 다루어지면서 발생하던 문제를 해결하기 위해 타입을 number로 교정하는 변경사항을 담고 있습니다. 전반적으로 좋은 방향의 수정이지만, clubId를 숫자로 변환하는 과정에서 발생할 수 있는 엣지 케이스(edge case)에 대한 처리가 필요해 보입니다. Page.tsx에서 Number() 함수 사용 시 NaN이 될 수 있는 경우와 0을 처리하는 부분에 대한 리뷰 의견을 남겼습니다. 이 부분을 수정하면 코드가 더 안정적으로 동작할 것입니다.
| if (!clubId) return; | ||
| fetchClubDetail(clubId).then(setClub).catch(console.error); | ||
| }, [clubId]); | ||
| if (!clubIdNumber) return; |
There was a problem hiding this comment.
현재의 !clubIdNumber 조건은 clubId가 undefined이거나 숫자가 아닌 문자열일 때 NaN이 되는 경우를 제대로 처리하지 못합니다 (!NaN은 false). 또한 clubId가 '0'일 경우 0으로 변환되어 !0이 true가 되므로, 유효한 ID임에도 불구하고 데이터를 가져오지 못하는 버그가 발생할 수 있습니다.
isNaN을 사용하여 clubIdNumber가 유효한 숫자인지 확인하는 것이 더 안전합니다.
| if (!clubIdNumber) return; | |
| if (isNaN(clubIdNumber)) return; |
![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
🧹 Nitpick comments (1)
src/pages/user/ClubDetail/Page.tsx (1)
18-18: Prefer explicit validation for numeric conversion.
Number(clubId)silently converts invalid inputs toNaN. While the guard at line 22 catches this, an explicit check improves clarity.Consider this alternative that validates before conversion:
- const clubIdNumber = Number(clubId); + const clubIdNumber = clubId ? Number(clubId) : NaN;Or combine with the improved guard:
const { clubId } = useParams<{ clubId: string }>(); - const clubIdNumber = Number(clubId); + const clubIdNumber = clubId ? Number(clubId) : undefined; const [club, setClub] = useState<ClubDetail | null>(null); useEffect(() => { - if (!clubIdNumber) return; + if (clubIdNumber === undefined || Number.isNaN(clubIdNumber)) return; fetchClubDetail(clubIdNumber).then(setClub).catch(console.error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/user/ClubDetail/Page.tsx(1 hunks)src/pages/user/ClubDetail/components/ClubInfoSidebarSection/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/user/ClubDetail/Page.tsx (2)
src/pages/user/ClubDetail/types/clubDetail.ts (1)
ClubDetail(3-20)src/pages/user/ClubDetail/api/clubDetail.ts (1)
fetchClubDetail(5-9)
🔇 Additional comments (2)
src/pages/user/ClubDetail/components/ClubInfoSidebarSection/index.tsx (1)
17-17: LGTM! Type narrowing improves type safety.Narrowing
clubIdfromnumber | stringtonumbereliminates ambiguity and aligns with the fix objective. The URL construction at line 41 correctly coerces the number to a string in the template literal.src/pages/user/ClubDetail/Page.tsx (1)
24-24: Effect dependency is correct.Changing the dependency from
[clubId]to[clubIdNumber]is appropriate since the effect now usesclubIdNumberfor the fetch call. Primitive number values are compared by value, so the dependency will trigger correctly when the route parameter changes.
| if (!clubId) return; | ||
| fetchClubDetail(clubId).then(setClub).catch(console.error); | ||
| }, [clubId]); | ||
| if (!clubIdNumber) return; |
There was a problem hiding this comment.
Guard rejects valid club ID 0.
The guard if (!clubIdNumber) is truthy for 0, NaN, null, undefined, and false. While it correctly catches NaN from invalid conversions, it also incorrectly rejects a club ID of 0 if that's a valid identifier in your system.
Apply this diff to check for NaN explicitly:
- if (!clubIdNumber) return;
+ if (!clubId || Number.isNaN(clubIdNumber)) return;This way, club ID 0 is accepted while invalid conversions are still caught.
📝 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.
| if (!clubIdNumber) return; | |
| // replace this: | |
| - if (!clubIdNumber) return; | |
| // with this: | |
| if (!clubId || Number.isNaN(clubIdNumber)) return; |
🤖 Prompt for AI Agents
In src/pages/user/ClubDetail/Page.tsx around line 22, the guard `if
(!clubIdNumber) return;` wrongly rejects a valid club ID of 0; change the check
to explicitly test for NaN (e.g., use Number.isNaN(clubIdNumber) or
isNaN(clubIdNumber)) so that only invalid numeric conversions are rejected while
allowing 0 to pass; update any related early-return behavior or types
accordingly to ensure downstream code receives a valid numeric ID.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mocks/repositories/club.ts (2)
338-348: Consider renaming parameter for clarity.The function parameter is named
id, but it's used to look upclub.clubId. For consistency and clarity, consider renaming the parameter toclubId.Apply this diff:
- updateClubDetail: (id: number, updatedData: Partial<ClubDetail>) => { - const index = mockClubDetail.findIndex((club) => club.clubId === id); + updateClubDetail: (clubId: number, updatedData: Partial<ClubDetail>) => { + const index = mockClubDetail.findIndex((club) => club.clubId === clubId);Similarly, consider updating
getClubDetailById:- getClubDetailById: (id: number) => { - return mockClubDetail.find((club) => club.clubId === id); + getClubDetailById: (clubId: number) => { + return mockClubDetail.find((club) => club.clubId === clubId);
334-336: Unify identifier property names between Club and ClubDetail: Club usesidbut ClubDetail usesclubId; consider aligning these to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/mocks/repositories/club.ts(4 hunks)src/pages/user/ClubDetail/Page.tsx(2 hunks)src/pages/user/ClubDetail/api/clubDetail.ts(1 hunks)src/pages/user/ClubDetail/components/ClubInfoSidebarSection/index.tsx(2 hunks)src/pages/user/ClubDetail/types/clubDetail.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/user/ClubDetail/Page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/user/ClubDetail/api/clubDetail.ts (1)
src/pages/user/ClubDetail/types/clubDetail.ts (1)
ClubDetail(3-20)
src/mocks/repositories/club.ts (1)
src/pages/user/ClubDetail/types/clubDetail.ts (1)
ClubDetail(3-20)
🔇 Additional comments (5)
src/mocks/repositories/club.ts (1)
242-242: Mock data updated correctly.The mock data entries correctly use
clubIdinstead ofid, aligning with theClubDetailtype definition.Also applies to: 271-271, 299-299
src/pages/user/ClubDetail/components/ClubInfoSidebarSection/index.tsx (2)
6-17: Improved type safety using Pick.Using
Pick<ClubDetail, ...>to derive the props ensures thatclubIdmaintains the same type as defined inClubDetail(number), improving type consistency across the component tree. This change directly addresses the PR objective of ensuringclubIdis typed as a number, preventing the undefined path issue.
40-40: Apply button route now correctly uses numeric clubId.The route construction now uses
clubIdas a number (via theClubDetailtype), which should resolve the issue where the club ID appeared asundefinedin the URL path. The template literal will properly convert the number to a string for the URL.src/pages/user/ClubDetail/api/clubDetail.ts (1)
5-5: fetchClubDetail parameter narrowing is safe
Verified that src/pages/user/ClubDetail/Page.tsx convertsuseParams’sclubIdto a number before callingfetchClubDetail. No other call sites found.src/pages/user/ClubDetail/types/clubDetail.ts (1)
4-4: LGTM – no residualClubDetail.idusages found; all remainingclub.idrefs are on the separateClubtype.
…ly-button-path#158 [FIX] clubId 가 string이지 못하게 교정 (#158)
…ly-button-path#158 [FIX] clubId 가 string이지 못하게 교정 (#158)
3 participants
#️⃣연관된 이슈
close #158
📝작업 내용
지원하기 버튼을 눌러 이동할 때 경로의 클럽아이디가 언디파인드로 뜨는 문제
-> 클럽 아이디를 스트링으로 받지 못하도록 교정 및 타입을 유니온이 아니게 수정
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor