Dev/announcement #101
Dev/announcement #101
Conversation
WalkthroughApp adds noticeTitle/noticeContent state and a handler to open a "Notice" modal. Notice items call that handler (preventing navigation when content exists). Modal and ModalOpen accept a Notice target and render a "공지사항" header and the passed title/content. NoticeInfo type adds a content field. Changes
Sequence DiagramsequenceDiagram
actor User
participant Notice as Notice Component
participant App as App Component
participant Modal as Modal Component
participant ModalOpen as ModalOpen Component
User->>Notice: Click notice item
Note over Notice: item has `content`
Notice->>App: onModalOpen(content, title)
App->>App: set noticeContent/noticeTitle, mTarget='Notice'
App->>Modal: render with mTarget='Notice'
App->>ModalOpen: pass noticeContent, noticeTitle
Modal->>Modal: render "공지사항" header and set overflow-hidden
ModalOpen->>ModalOpen: render title (if any) and noticeContent as plain text
Modal->>User: display notice modal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/components/notice/Notice.tsx (1)
7-25: Tighten Notice click logic to avoid empty-modal edge casesThe click handler currently does:
if (props.content.trim() !== '' || props.url.trim() === '') { e.preventDefault() props.onModalOpen(props.content, props.title) }This means:
- If
contentis non-empty: navigation is prevented and the modal opens (expected).- If both
contentandurlare empty: navigation is still prevented but an empty modal opens.- If
contentis empty buturlis non-empty: the link navigates as before, which may or may not match the “no longer navigates to a link” behavior described in the PR summary.If the intended behavior is “open modal only when there is content; otherwise, navigate when a URL exists and do nothing when there isn’t”, a safer pattern would be:
const handleClick = (e: React.MouseEvent<HTMLAnchorElement>) => { const hasContent = props.content.trim() !== '' const hasUrl = props.url.trim() !== '' if (hasContent) { e.preventDefault() props.onModalOpen(props.content, props.title) } else if (!hasUrl) { e.preventDefault() // nothing to open or navigate to } }Also, since this is now primarily a “show modal” interaction, you may want to reconsider using
cursor-defaulton the<a>so it still feels clickable, or switch to a<button>-like control for better semantics.
🧹 Nitpick comments (5)
src/app/data/notice/noticeInfo.ts (1)
3-8: Verifycontentis always present in notice dataMaking
contenta requiredstringmeans all producers ofNoticeInfo(API responses, fixtures, etc.) must now supply it. If any item comes back withoutcontent, consumers likeNotice.tsxcallingitem.content.trim()will throw at runtime.Either ensure the backend/schema always returns a string (possibly
'') forcontent, or consider typing this ascontent?: stringand defaulting to''at the edge when mapping the API response.src/app/components/modal/modal.tsx (2)
31-38: ModalSection overflow behavior looks correct; consider tightening$mTargettypeThe conditional
overflow-hiddenfor'Notice'vsoverflow-autofor others matches the new design and avoids the previous always-on scroll.For extra safety, you could narrow
$mTarget(andmTargetonModal) to a string union of known targets ('Fabs' | 'Info' | 'Christmas' | 'Spring' | 'Notice') instead of a genericstring, so typos are caught at compile time.
95-111: Align Notice modal header with i18n usageOther modal headers (
Fabs,Info,Christmas,Spring) are localized viat(...), but the Notice header uses a hard-coded"공지사항"string. For consistency and future localization, consider adding a translation key (e.g.t('notice')) instead of the literal.src/app/components/modal/modalOpen.tsx (1)
59-65: Consider moving inline overflow styles into styled-componentsThe conditional overflow logic for
'Notice'vs other targets is fine, but it duplicates intent with theModalSection’s$mTarget-based overflow and uses inline styles.If this grows more complex, consider expressing it via a styled wrapper that takes
$mTarget(or a dedicated$scrollableflag) instead of inlinestyle, to keep layout concerns co-located in styled-components.src/App.tsx (1)
196-232: Notice modal state and open handler are straightforward
noticeContent/noticeTitlestate plushandleNoticeModalOpencleanly encapsulate the Notice modal behavior and reuse the existing modal infrastructure. The call order (setNotice*,setModalTarget('Notice'),openModal()) is fine given React’s batched updates.If you expect more Notice-specific modal data later, you might eventually group these into a single
noticeModalstate object, but that’s not necessary right now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/App.tsx(4 hunks)src/app/components/modal/modal.tsx(3 hunks)src/app/components/modal/modalOpen.tsx(3 hunks)src/app/components/notice/Notice.tsx(3 hunks)src/app/data/notice/noticeInfo.ts(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.40.0)
src/app/components/modal/modalOpen.tsx
[warning] 119-119: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
src/app/components/modal/modalOpen.tsx
[error] 120-120: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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: Generate Lighthouse Report
🔇 Additional comments (4)
src/app/components/modal/modalOpen.tsx (1)
31-39: Notice props extension is consistentAdding
noticeContent?andnoticeTitle?here lines up cleanly with the new Notice modal flow fromApp.tsxandNotice.tsx. No functional concerns from the prop typing.src/App.tsx (2)
405-407: Notice → App callback wiring looks goodPassing
onModalOpen={handleNoticeModalOpen}intoNoticecleanly delegates the “what to do on click” decision back toApp, matching the new modal-driven behavior without couplingNoticeto modal internals.
582-590: ModalOpen receives all required Notice props
ModalOpennow getsnoticeContentandnoticeTitlealongsidemTarget, which matches its new prop signature and Notice branch. No additional wiring issues spotted here.src/app/components/notice/Notice.tsx (1)
37-67: Notice component correctly forwards modal callback and contentWiring
onModalOpenthroughNoticedown intoBox, along withitem.content, gives you a clean data flow from the fetched notices into the modal system without coupling toAppinternals. This matches the new modal-based UX for notices.
Deploying hybus-genesis with
|
| Latest commit: |
069a423
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://389d50e4.hybus-genesis.pages.dev |
| Branch Preview URL: | https://dev-announcement.hybus-genesis.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/components/modal/modalOpen.tsx(3 hunks)
⏰ 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). (2)
- GitHub Check: Generate Lighthouse Report
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/app/components/modal/modalOpen.tsx (2)
37-38: LGTM: Props are well-defined.The optional
noticeContentandnoticeTitleprops are properly typed and maintain backward compatibility.
111-122: Notice content renders as plain text (XSS-safe).The implementation correctly renders notice content as plain text
{props.noticeContent || ''}, which is safe from XSS attacks. The noticeContent prop is a simple string with no HTML formatting support, and React's default text escaping prevents injection attacks. Thewhitespace-pre-wrapCSS class appropriately preserves line breaks and spaces in the plain text content without enabling HTML rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/components/modal/modalOpen.tsx (1)
31-39: Notice props extension looks correct; consider tightening typing aroundmTarget.The new optional
noticeContent/noticeTitleprops are wired cleanly and only used whenmTarget === 'Notice'. For future robustness, you might later consider makingmTargeta discriminated union (e.g.'Fabs' | 'Info' | 'Christmas' | 'Spring' | 'Notice') and tyingnoticeContent/noticeTitleto the'Notice'case so TS enforces correct usage, but this is non-blocking for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/components/modal/modalOpen.tsx(3 hunks)
⏰ 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). (2)
- GitHub Check: Generate Lighthouse Report
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
src/app/components/modal/modalOpen.tsx (2)
59-62: Unified scrollable content container improves long‑content handling.Switching to a single
style={{ overflow: 'auto', maxHeight: '450px' }}for all targets resolves the previous Notice scrolling issue while keeping other modals unchanged. This is a good, simple UX fix.
107-118: Notice body rendering is safe and matches the new behavior.The Notice branch cleanly renders an optional title and uses
whitespace-pre-wrapwith a plain text{props.noticeContent || ''}node, which both preserves basic formatting and avoids XSS risks from HTML content. This aligns well with “open modal and show notice content” behavior.
|
LGTM |
|
Check out your Lighthouse Report: https://lighthouse.hybus.app/app/projects/bushanyang-production/dashboard |
Describe your Pull Request
When a notice is clicked, instead of navigating to a link as before, it now opens a modal if the notice has content, and the content is displayed inside the modal.
Please describe the behavior or changes of your code specifically.
Additional content
Any additional comments? 😁
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.