Repository navigation
26.03.23 최신화 - #238
26.03.23 최신화#238
Conversation
📝 WalkthroughWalkthroughAdded site icon metadata and routing for notifications: new utility maps notification types to destination URLs; notification UI now marks as read then navigates via Next.js router. Also added a new external NEWS_FROM_URL and updated two CTA handlers. Changes
Sequence DiagramsequenceDiagram
actor User
participant Component as Notification Component
participant Utility as getNotificationRedirectUrl
participant Router as Next.js Router
participant Target as Target Page
User->>Component: Click notification
Component->>Utility: getNotificationRedirectUrl(notification)
Utility->>Utility: Resolve URL by notificationType
Utility-->>Component: Return destination URL
Component->>Component: Mark notification as read (if unread)
Component->>Router: router.push(redirectUrl)
Router->>Target: Navigate to destination
Target-->>User: Display destination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, 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
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
![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
안녕하세요. 알림 클릭 시 관련 페이지로 리디렉션하는 기능 추가와 아이콘 설정 업데이트를 확인했습니다. 전반적으로 좋은 개선입니다.
리뷰 중 몇 가지 개선점을 발견했습니다:
MyNotificationItem과NotificationDropdown컴포넌트에서 알림 클릭 처리 로직이 중복됩니다. 유지보수성을 위해 이를 커스텀 훅으로 추출하는 것을 제안합니다.layout.tsx에 추가된 Apple 아이콘 경로에 파일 확장자가 누락된 것으로 보입니다. 이로 인해 아이콘이 제대로 표시되지 않을 수 있습니다.
자세한 내용은 각 파일에 남긴 코멘트를 참고해주세요.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/app/layout.tsx (1)
36-39: Remove duplicated metadata icons (or make Apple icon path explicit if keeping config).
src/app/icon.svgandsrc/app/apple-icon.pngare auto-discovered special files in Next.js App Router and automatically added as metadata icons. Keeping both file-based metadata and expliciticonsconfig is redundant and can drift over time. Additionally,apple: "/apple-icon"should include the.pngextension ("/apple-icon.png") to match the standard format.♻️ Suggested simplification
Remove the icons block entirely:
export const metadata: Metadata = { - icons: { - icon: "/icon.svg", - apple: "/apple-icon", - }, };Or, if explicit config is preferred, fix the apple path:
icons: { icon: "/icon.svg", - apple: "/apple-icon", + apple: "/apple-icon.png", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/layout.tsx` around lines 36 - 39, The icons metadata block defining icons: { icon: "/icon.svg", apple: "/apple-icon" } is redundant with Next.js auto-discovered app router files and the apple path is missing its extension; either remove the entire icons block from layout.tsx to rely on auto-discovery, or if you prefer explicit metadata keep the icons object but change apple to "/apple-icon.png" and ensure the icon path matches the actual asset (icon: "/icon.svg"); update the icons object in layout.tsx accordingly.src/components/layout/NotificationDropdown.tsx (1)
16-22: Consider extracting shared notification click behavior.This read-then-redirect logic is duplicated with
src/components/base-ui/MyPage/items/MyNotificationItem.tsx(Line 20-26). A shared helper/hook would reduce drift risk if redirect/read rules change again.Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/NotificationDropdown.tsx` around lines 16 - 22, Extract the duplicated "read then redirect" behavior into a single helper/hook (e.g., useNotificationClick or handleNotificationClickHelper) and replace the inline logic in NotificationDropdown's handleNotificationClick and MyNotificationItem's click handler with a call to that shared function; the helper should accept a NotificationBasicInfo (or notificationId and notification object), call readNotification(notification.notificationId) only when notification.read is false, compute the redirect URL via getNotificationRedirectUrl(notification), and then call router.push(redirectUrl) (or return the URL for callers to navigate) so both components reuse the same behavior and avoid drift.src/utils/notification.ts (1)
23-37: Prefer exhaustive switch handling to catch new notification types at compile time.Current
defaultfallback works, but it can silently mask missing redirects whennotificationTypegrows. Add an exhaustivenevercheck so new types fail fast during development.♻️ Suggested refactor
export const getNotificationRedirectUrl = (notification: NotificationBasicInfo): string => { switch (notification.notificationType) { case "LIKE": case "COMMENT": return `/stories/${notification.domainId}`; case "FOLLOW": return `/profile/${encodeURIComponent(notification.displayName)}`; case "JOIN_CLUB": case "CLUB_MEETING_CREATED": case "CLUB_NOTICE_CREATED": return `/groups/${notification.domainId}`; - default: - return "/profile/mypage?tab=notifications"; } + + const _exhaustiveCheck: never = notification.notificationType; + return "/profile/mypage?tab=notifications"; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/notification.ts` around lines 23 - 37, The switch in getNotificationRedirectUrl currently uses a default fallback that can hide missing cases; replace the default branch with an exhaustive check so the compiler errors when NotificationBasicInfo.notificationType gains a new literal. Update getNotificationRedirectUrl to handle all known notificationType cases explicitly and, instead of returning a generic default, add an exhaustive-check (e.g., call an assertUnreachable or assign the remaining value to a variable typed as never) that throws an error including the unknown notification type; this ensures new notification types fail at compile time and during development rather than being silently routed to the generic notifications page.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/layout.tsx`:
- Around line 36-39: The icons metadata block defining icons: { icon:
"/icon.svg", apple: "/apple-icon" } is redundant with Next.js auto-discovered
app router files and the apple path is missing its extension; either remove the
entire icons block from layout.tsx to rely on auto-discovery, or if you prefer
explicit metadata keep the icons object but change apple to "/apple-icon.png"
and ensure the icon path matches the actual asset (icon: "/icon.svg"); update
the icons object in layout.tsx accordingly.
In `@src/components/layout/NotificationDropdown.tsx`:
- Around line 16-22: Extract the duplicated "read then redirect" behavior into a
single helper/hook (e.g., useNotificationClick or handleNotificationClickHelper)
and replace the inline logic in NotificationDropdown's handleNotificationClick
and MyNotificationItem's click handler with a call to that shared function; the
helper should accept a NotificationBasicInfo (or notificationId and notification
object), call readNotification(notification.notificationId) only when
notification.read is false, compute the redirect URL via
getNotificationRedirectUrl(notification), and then call router.push(redirectUrl)
(or return the URL for callers to navigate) so both components reuse the same
behavior and avoid drift.
In `@src/utils/notification.ts`:
- Around line 23-37: The switch in getNotificationRedirectUrl currently uses a
default fallback that can hide missing cases; replace the default branch with an
exhaustive check so the compiler errors when
NotificationBasicInfo.notificationType gains a new literal. Update
getNotificationRedirectUrl to handle all known notificationType cases explicitly
and, instead of returning a generic default, add an exhaustive-check (e.g., call
an assertUnreachable or assign the remaining value to a variable typed as never)
that throws an error including the unknown notification type; this ensures new
notification types fail at compile time and during development rather than being
silently routed to the generic notifications page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5652750b-814b-472c-b3aa-c208b2db32c7
⛔ Files ignored due to path filters (3)
src/app/apple-icon.pngis excluded by!**/*.pngsrc/app/favicon.icois excluded by!**/*.icosrc/app/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
src/app/layout.tsxsrc/components/base-ui/MyPage/items/MyNotificationItem.tsxsrc/components/layout/NotificationDropdown.tsxsrc/utils/notification.ts
fix: URL 링크 추가 및 수정
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/app/`(main)/news/page.tsx:
- Line 108: The onClick handler calls window.open(EXTERNAL_LINKS.NEWS_FROM_URL,
"_blank") which allows the opened page to access window.opener; change the call
to include the no-opener/no-referrer feature string (e.g.,
window.open(EXTERNAL_LINKS.NEWS_FROM_URL, "_blank", "noopener,noreferrer")) so
the new tab cannot access the opener. Update the onClick where window.open is
used (referenced by the onClick handler and EXTERNAL_LINKS.NEWS_FROM_URL) to
pass the "noopener,noreferrer" feature flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 106c4a6c-bb3a-4b3a-891d-a7cf6846ef5f
📒 Files selected for processing (3)
src/app/(main)/news/page.tsxsrc/components/base-ui/MyPage/ProfileSection/UserProfile.tsxsrc/constants/links.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/base-ui/MyPage/ProfileSection/UserProfile.tsx
- src/constants/links.ts
feat:모임 리다이렉션 로직 구현
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/components/base-ui/MyPage/items/MyMeetingCard.tsx`:
- Around line 39-42: The clickable card div in MyMeetingCard.tsx currently uses
onClick(() => router.push(`/groups/${club.clubId}`)) but is not
keyboard-accessible; make it focusable and operable via keyboard by either
replacing the div with a semantic element (e.g., a button or anchor) or adding
tabIndex={0}, role="button", and an onKeyDown handler that calls the same
router.push(`/groups/${club.clubId}`) when Enter or Space is pressed (prevent
default for Space), and include a descriptive aria-label if the surrounding
content isn't explicit; update the element with these changes so keyboard-only
users can open the meeting detail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07c938e4-159f-4965-bf6e-33cb624b4342
📒 Files selected for processing (1)
src/components/base-ui/MyPage/items/MyMeetingCard.tsx
| <div | ||
| onClick={() => router.push(`/groups/${club.clubId}`)} | ||
| className="flex w-full px-[18px] py-[12px] md:p-[20px] justify-between items-center rounded-[8px] bg-white border border-[#EAE5E2] gap-[12px] cursor-pointer hover:bg-gray-50 transition-colors" | ||
| > |
There was a problem hiding this comment.
Add keyboard-accessible semantics to the clickable card container.
Line 39 makes a div act like a navigation control, but it is not keyboard-focusable and has no keyboard activation handler. This blocks keyboard-only users from opening the meeting detail.
Suggested fix
- <div
- onClick={() => router.push(`/groups/${club.clubId}`)}
+ <div
+ role="button"
+ tabIndex={0}
+ onClick={() => router.push(`/groups/${club.clubId}`)}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ router.push(`/groups/${club.clubId}`);
+ }
+ }}
className="flex w-full px-[18px] py-[12px] md:p-[20px] justify-between items-center rounded-[8px] bg-white border border-[`#EAE5E2`] gap-[12px] cursor-pointer hover:bg-gray-50 transition-colors"
>📝 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.
| <div | |
| onClick={() => router.push(`/groups/${club.clubId}`)} | |
| className="flex w-full px-[18px] py-[12px] md:p-[20px] justify-between items-center rounded-[8px] bg-white border border-[#EAE5E2] gap-[12px] cursor-pointer hover:bg-gray-50 transition-colors" | |
| > | |
| <div | |
| role="button" | |
| tabIndex={0} | |
| onClick={() => router.push(`/groups/${club.clubId}`)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| router.push(`/groups/${club.clubId}`); | |
| } | |
| }} | |
| className="flex w-full px-[18px] py-[12px] md:p-[20px] justify-between items-center rounded-[8px] bg-white border border-[`#EAE5E2`] gap-[12px] cursor-pointer hover:bg-gray-50 transition-colors" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/base-ui/MyPage/items/MyMeetingCard.tsx` around lines 39 - 42,
The clickable card div in MyMeetingCard.tsx currently uses onClick(() =>
router.push(`/groups/${club.clubId}`)) but is not keyboard-accessible; make it
focusable and operable via keyboard by either replacing the div with a semantic
element (e.g., a button or anchor) or adding tabIndex={0}, role="button", and an
onKeyDown handler that calls the same router.push(`/groups/${club.clubId}`) when
Enter or Space is pressed (prevent default for Space), and include a descriptive
aria-label if the surrounding content isn't explicit; update the element with
these changes so keyboard-only users can open the meeting detail.
📌 개요 (Summary)
🛠️ 변경 사항 (Changes)
📸 스크린샷 (Screenshots)
(UI 변경 사항이 있다면 첨부해주세요)
✅ 체크리스트 (Checklist)
pnpm build)pnpm lint)Summary by CodeRabbit
New Features
Chores