Refactor(ads): 저장한 공지 이미지 없을 때 기본 이미지 처리#280
Open
jin-evergreen wants to merge 3 commits intodevelopfrom
Open
Conversation
📝 WalkthroughSummary by CodeRabbit릴리스 노트
Walkthrough빈 상태 이미지 자산을 추가하고, 카드 공지 컴포넌트의 이미지 소스와 에러 핸들링을 변경했으며, 공지 컨테이너 스타일에 너비와 정렬 속성을 추가했습니다. 버튼으로의 역할 변경과 접근성 속성도 포함됩니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx`:
- Line 1: The import in card-notice.tsx uses a relative path import of IMAGES
from '../../../assets' which violates the repo convention; change it to the
project's absolute alias import (e.g., replace the relative import with the
appropriate alias import that exposes IMAGES such as `@shared/assets` or `@assets`
depending on your alias map) so the IMAGES symbol is imported via the project's
configured path alias used across {apps,packages}/**/src/**.
- Line 26: The clickable article element in the CardNotice component lacks
keyboard accessibility; update the <article> (the element rendered in
card-notice.tsx) to be focusable and keyboard-activatable by adding
role="button", tabIndex={0}, and an onKeyDown handler that calls the existing
onClick when Enter (key === "Enter") or Space (key === " " or key ===
"Spacebar") is pressed; ensure the onKeyDown handler prevents default for Space
to avoid page scroll and preserves the existing onClick behavior so both mouse
and keyboard activate the same action.
- Around line 31-34: Replace the inline anonymous onError handler in
CardNotice's JSX with a named event handler (e.g., handleImageError) to follow
the 'handle*' convention; implement a function (handleImageError) in the
component that accepts the event (React.SyntheticEvent<HTMLImageElement, Event>
or appropriate type), sets e.currentTarget.onerror = null and
e.currentTarget.src = IMAGES.EMPTY_NOTICE_IMAGE, and then reference this handler
in the JSX as onError={handleImageError}; ensure the handler is declared in the
same component scope so it can access IMAGES and any props/state.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/ads-ui/src/assets/empty-notice-image.webpis excluded by!**/*.webpand included bypackages/**/src/**
📒 Files selected for processing (2)
packages/ads-ui/src/assets/index.tspackages/ads-ui/src/components/card/card-notice/card-notice.tsx
| @@ -1,3 +1,4 @@ | |||
| import { IMAGES } from '../../../assets'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suggest: Line 1 상대 경로 import를 절대 경로 alias로 통일해 주세요.
새로 추가된 import가 상대 경로(../../../assets)라서 레포 컨벤션과 어긋납니다. 프로젝트에서 사용하는 alias(@pages, @shared 등) 기준으로 바꿔주시면 유지보수성이 좋아집니다.
As per coding guidelines {apps,packages}/**/src/**: 절대 경로 임포트 사용 (@pages, @shared 등).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx` at line 1,
The import in card-notice.tsx uses a relative path import of IMAGES from
'../../../assets' which violates the repo convention; change it to the project's
absolute alias import (e.g., replace the relative import with the appropriate
alias import that exposes IMAGES such as `@shared/assets` or `@assets` depending on
your alias map) so the IMAGES symbol is imported via the project's configured
path alias used across {apps,packages}/**/src/**.
packages/ads-ui/src/components/card/card-notice/card-notice.tsx
Outdated
Show resolved
Hide resolved
packages/ads-ui/src/components/card/card-notice/card-notice.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ads-ui/src/components/card/card-notice/card-notice.tsx (1)
3-3:⚠️ Potential issue | 🟠 MajorMust: Line 3 import를 절대 경로 alias로 통일해 주세요.
현재
../../../assets상대 경로 import는 공용 코드 컨벤션과 맞지 않습니다. 프로젝트 alias 기준의 절대 경로 import로 변경 부탁드립니다.As per coding guidelines
{apps,packages}/**/src/**:절대 경로 임포트 사용 (@pages,@shared등).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx` at line 3, Replace the relative import for IMAGES (currently importing from '../../../assets') with the project's absolute alias import per repo conventions (e.g., use the configured alias such as `@assets`, `@shared/assets`, or `@shared`) so the line becomes an absolute-path import; update the import that references IMAGES to use the correct project alias and ensure any export names remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx`:
- Around line 40-47: The article currently assigns interactive/a11y attributes
unconditionally (onClick, onKeyDown, role='button', tabIndex={0}) which makes
non-clickable items focusable and announce as buttons; change the component to
apply these attributes only when clickable (e.g., when the onClick prop exists
or an explicit isClickable prop is true): attach onClick and onKeyDown
(handleKeyDown) conditionally, set role='button' and tabIndex=0 only for
clickable instances and omit or set tabIndex={-1} otherwise, while keeping
non-interactive markup for the rest (title/aria-label can remain); update
references in card-notice (props handling and JSX where onClick, handleKeyDown,
role, and tabIndex are set) to implement this conditional logic.
---
Duplicate comments:
In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx`:
- Line 3: Replace the relative import for IMAGES (currently importing from
'../../../assets') with the project's absolute alias import per repo conventions
(e.g., use the configured alias such as `@assets`, `@shared/assets`, or `@shared`) so
the line becomes an absolute-path import; update the import that references
IMAGES to use the correct project alias and ensure any export names remain
unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/ads-ui/src/assets/.pngis excluded by!**/*.png,!**/*.pngand included bypackages/**/src/**packages/ads-ui/src/assets/img_empty_alert.pngis excluded by!**/*.png,!**/*.pngand included bypackages/**/src/**
📒 Files selected for processing (1)
packages/ads-ui/src/components/card/card-notice/card-notice.tsx
packages/ads-ui/src/components/card/card-notice/card-notice.tsx
Outdated
Show resolved
Hide resolved
Sohyunnnn
approved these changes
Mar 1, 2026
Contributor
Sohyunnnn
left a comment
Sohyunnnn
left a comment
There was a problem hiding this comment.
수고하셨습니다 👍
이미지 로드 실패 케이스까지 onError로 방어해주신 점 좋았습니다!!
jisooooooooooo
approved these changes
Mar 2, 2026
Contributor
jisooooooooooo
left a comment
jisooooooooooo
left a comment
There was a problem hiding this comment.
확인했습니당 !!
토끼 리뷰랑 소현님 리뷰만 고려해 보면 좋을 것 같네용ㅎㅎ😙
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/ads-ui/src/components/card/card-notice/card-notice.tsx (2)
3-3:⚠️ Potential issue | 🟠 MajorMust: 상대 경로 import를 절대 경로 alias로 통일해 주세요.
Line 3의 상대 경로 import는 현재 레포 컨벤션과 맞지 않습니다. 패키지 내 표준 alias 경로로 변경 부탁드립니다.
As per coding guidelines
{apps,packages}/**/src/**:절대 경로 임포트 사용 (@pages,@shared등).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx` at line 3, The import in card-notice.tsx uses a relative path (import { IMAGES } from '../../../assets';) which violates the repo convention; update it to the package's absolute alias import (e.g., import { IMAGES } from '@assets' or the project's defined alias for assets) so it follows the {apps,packages}/**/src/** absolute import rule (use the same alias pattern used elsewhere like `@pages` or `@shared`).
33-38:⚠️ Potential issue | 🟠 MajorMust:
onClick이 없을 때는 비인터랙티브 상태를 명확히 처리해 주세요.Line 33-38에서
onClick이 없는 경우에도 버튼과aria-label이 유지되어 사용자에게 “클릭 가능”으로 전달됩니다.onClick을 필수로 만들거나, 없을 때disabled/라벨 조건부 처리를 권장드립니다.제안 diff
const CardNotice = ({ imageUrl, title, content, isPinned, createdAt, onClick, }: CardNoticeProps) => { + const isClickable = Boolean(onClick); const displayImage = imageUrl || IMAGES.EMPTY_NOTICE_IMAGE; const handleImageError = (event: SyntheticEvent<HTMLImageElement>) => { event.currentTarget.onerror = null; event.currentTarget.src = IMAGES.EMPTY_NOTICE_IMAGE; }; return ( <button className={styles.notice} - onClick={onClick} - aria-label={`${title} 상세 보기`} + onClick={isClickable ? onClick : undefined} + disabled={!isClickable} + aria-label={isClickable ? `${title} 상세 보기` : undefined} type='button' >As per coding guidelines
packages/ads-ui/src/**:접근성(a11y)과 토큰 일관성 우선 검토.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx` around lines 33 - 38, The CardNotice button currently presents as interactive even when onClick is not provided; update the CardNotice rendering logic (the element using className={styles.notice}, props onClick and title) so that when onClick is undefined it is rendered as non-interactive: either render a non-interactive element (e.g., a <div> or <span>) or render the <button> with disabled and aria-disabled="true" and remove/adjust the interactive aria-label; ensure the accessible label reflects the non-interactive state and that styles.notice still applies, or alternatively make onClick a required prop on the CardNotice component to guarantee interactivity consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ads-ui/src/components/card/card-notice/card-notice.tsx`:
- Line 3: The import in card-notice.tsx uses a relative path (import { IMAGES }
from '../../../assets';) which violates the repo convention; update it to the
package's absolute alias import (e.g., import { IMAGES } from '@assets' or the
project's defined alias for assets) so it follows the {apps,packages}/**/src/**
absolute import rule (use the same alias pattern used elsewhere like `@pages` or
`@shared`).
- Around line 33-38: The CardNotice button currently presents as interactive
even when onClick is not provided; update the CardNotice rendering logic (the
element using className={styles.notice}, props onClick and title) so that when
onClick is undefined it is rendered as non-interactive: either render a
non-interactive element (e.g., a <div> or <span>) or render the <button> with
disabled and aria-disabled="true" and remove/adjust the interactive aria-label;
ensure the accessible label reflects the non-interactive state and that
styles.notice still applies, or alternatively make onClick a required prop on
the CardNotice component to guarantee interactivity consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/ads-ui/src/components/card/card-notice/card-notice.css.tspackages/ads-ui/src/components/card/card-notice/card-notice.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
저장한 공지확인 시 공지 이미지가 없을 때 기본 이미지 처리 로직을 추가합니다.📚 Tasks
webp) 추가🔍 Describe
공지 이미지 없을 때 이미지 깨짐 해결
공연 공지는 이미지가 필수 입력 값이 아니므로, 이미지가 없는 경우도 존재해요. 기존에는
저장한 공지확인 시 공지사항 데이터 중 이미지 URL이 비어있거나, 이미지를 불러오지 못할 경우 엑스박스가 노출되며 이미지가 깨지는 현상이 발생했어요.이를 해결하기 위해 렌더링 시점에
imageUrl이 존재하지 않으면 기본 이미지를 렌더링하도록 했어요. 추가로,imageUrl값은 존재하지만 이미지 로드 실패, 파일 손상, 브라우저 미지원 형식 등으로 이미지를 불러올 수 없는 경우에도img태그의onError를 활용하여 기본 이미지가 렌더링 될 수 있도록 했어요.👀 To Reviewer
📸 Screenshot