Feature#371 배너 클릭시 지원하기 페이지로 이동하도록 수정 #372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces the banner container with a Next.js Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant BannerUI as Banner (Link)
participant NextLink as Next/Link
participant ApplyPage as /apply
rect rgba(200,240,255,0.25)
note over User,BannerUI: Click banner area (Link)
User->>BannerUI: Click
BannerUI->>NextLink: native link navigation
NextLink-->>ApplyPage: Navigate to /apply
end
User->>BannerUI: Click close (X)
note over BannerUI: close handler calls e.stopPropagation()
BannerUI->>BannerUI: hideBanner()
BannerUI--x NextLink: Navigation prevented
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
📝 추가 및 변경된 파일총 1개 파일 변경 |
|
📚 Storybook이 Chromatic에 배포되었습니다!
|
![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)
mosu-app/src/features/banner/ui/Banner.tsx (1)
56-57: Guard against SSR and missing portal target
document.getElementById(...)may be null or unavailable during SSR, causingcreatePortalto throw. Guard before calling.Use this pattern:
// at top import React from "react"; // inside component const [mountNode, setMountNode] = React.useState<HTMLElement | null>(null); React.useEffect(() => { setMountNode(document.getElementById(BANNER_PORTAL_ID) as HTMLElement | null); }, []); if (!isVisible || !mountNode) return null; return createPortal( /* ...section... */, mountNode, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mosu-app/src/features/banner/ui/Banner.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
mosu-app/src/features/banner/ui/Banner.tsx (1)
mosu-app/src/features/banner/contexts/BannerProvider.tsx (1)
useBannerVisibility(39-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run DangerJS
🔇 Additional comments (3)
mosu-app/src/features/banner/ui/Banner.tsx (3)
13-13: LGTM on router instantiation
No issues withconst router = useRouter();.
47-50: Good use of stopPropagation on the close buttonPrevents unintended navigation while closing. Looks correct.
3-3: No change needed: using Pages Router
Banner.tsx correctly importsuseRouterfromnext/router. The repository has noapp/directory undermosu-appand all other routes (includingsrc/pages/**) usenext/router, confirming the Pages Router is in use. Resolving this comment.
📚 Storybook이 Chromatic에 배포되었습니다!
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mosu-app/src/features/banner/ui/Banner.tsx (1)
3-3: LGTM: Using Next.js Link improves semantics and keyboard a11yReplacing the clickable container with Link is the right move and addresses the earlier a11y concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mosu-app/src/features/banner/ui/Banner.tsx(3 hunks)
🔇 Additional comments (1)
mosu-app/src/features/banner/ui/Banner.tsx (1)
19-26: Add Google Analytics event to Banner’s apply linkVerified: there’s no existing analytics helper in the codebase, so using
window.gtagdirectly is acceptable.• File:
mosu-app/src/features/banner/ui/Banner.tsx(around lines 19–26)
• Purpose: emit a “banner_click” GA event before client-side navigation to satisfy issue #371.Apply this diff (note corrected Tailwind class from
w/fulltow-full):--- a/mosu-app/src/features/banner/ui/Banner.tsx +++ b/mosu-app/src/features/banner/ui/Banner.tsx @@ -19,7 +19,12 @@ <Link - className="relative flex w/full justify-start px-4 hover:cursor-pointer md:justify-center" + className="relative flex w-full justify-start px-4 hover:cursor-pointer md:justify-center" style={{ height: BANNER_HEIGHT, background: "linear-gradient(to bottom, #1d1d1d28 0%, #ff1d3828 100%)", }} + onClick={() => { + window.gtag?.("event", "banner_click", { + event_category: "engagement", + event_label: "apply", + transport_type: "beacon", + }); + }} href="/apply" >Optional: If you encounter TypeScript errors about
window.gtag, add a declaration (e.g., inglobal.d.ts):declare global { interface Window { gtag?: (...args: any[]) => void; } }
| <button | ||
| className="absolute right-2 block h-full hover:cursor-pointer md:right-5" | ||
| aria-label="배너 닫기" | ||
| onClick={hideBanner} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| hideBanner(); | ||
| }} | ||
| > | ||
| <X color="#fff" /> | ||
| </button> | ||
| </div> | ||
| </Link> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix nested interactive control (button inside Link) and guard against accidental navigation
A button inside an anchor violates a11y (no-nested-interactive) and may still navigate despite stopPropagation. At minimum, prevent default; ideally, move the button outside Link and absolutely position it.
Minimal guard (within current structure):
- <button
+ <button
+ type="button"
className="absolute right-2 block h-full hover:cursor-pointer md:right-5"
aria-label="배너 닫기"
- onClick={(e) => {
- e.stopPropagation();
- hideBanner();
- }}
+ onClick={(e) => {
+ e.preventDefault();
+ e.stopPropagation();
+ hideBanner();
+ }}
>Preferred refactor (move button outside Link to avoid nested interactive):
- <section className="w-full bg-black" style={{ height: BANNER_HEIGHT }}>
- <Link
- className="relative flex w-full justify-start px-4 hover:cursor-pointer md:justify-center"
+ <section className="relative w-full bg-black" style={{ height: BANNER_HEIGHT }}>
+ <Link
+ className="flex w-full justify-start px-4 hover:cursor-pointer md:justify-center"
style={{
height: BANNER_HEIGHT,
background: "linear-gradient(to bottom, #1d1d1d28 0%, #ff1d3828 100%)",
}}
href="/apply"
+ onClick={() => {
+ (window as any).gtag?.("event", "banner_click", {
+ event_category: "engagement",
+ event_label: "apply",
+ transport_type: "beacon",
+ });
+ }}
>
...
- <button
- className="absolute right-2 block h-full hover:cursor-pointer md:right-5"
- aria-label="배너 닫기"
- onClick={(e) => {
- e.stopPropagation();
- hideBanner();
- }}
- >
- <X color="#fff" />
- </button>
- </Link>
+ </Link>
+ <button
+ type="button"
+ className="absolute inset-y-0 right-2 z-10 flex items-center hover:cursor-pointer md:right-5"
+ aria-label="배너 닫기"
+ onClick={() => hideBanner()}
+ >
+ <X color="#fff" />
+ </button>🤖 Prompt for AI Agents
In mosu-app/src/features/banner/ui/Banner.tsx around lines 43 to 53 there is a
button rendered inside a Link which creates a nested interactive control and can
still trigger navigation; fix it by either (preferred) moving the button outside
the Link and positioning it absolutely so it visually overlaps the banner while
the Link only wraps the non-interactive content, or (minimal) keep the button
inside but update its onClick to call e.preventDefault() and
e.stopPropagation(), then call hideBanner(), and ensure the button retains
aria-label and keyboard focusability; implement the preferred refactor where
possible to fully resolve the a11y violation.
✅ Linked Issue
🔍 What I did
Summary by CodeRabbit