Merged
Conversation
Walkthrough세 컴포넌트(DetailHeader, StoreCardInMultiPinList, StoreCardInSave)에서 북마크 아이콘 선택을 정적 isKeep 판정에서 새 getMarkerIcon 로직으로 변경하고 MultiSaveMarker SVG를 추가로 임포트하여 그룹 선택 수에 따라 아이콘을 결정하도록 했습니다. Changes
Sequence Diagram(s)(생성 기준 미충족 — 시각화 생략) Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 분 Possibly related PRs
Suggested reviewers
개요세 개의 UI 컴포넌트(DetailHeader, StoreCardInMultiPinList, StoreCardInSave)에서 북마크 아이콘 선택 로직을 정적 상태에서 동적 로직으로 변경했습니다. 새로운 getMarkerIcon 함수를 추가하여 선택된 그룹 수에 따라 적절한 아이콘을 렌더링하며, MultiSaveMarker 자산을 새로 임포트했습니다. 변경사항
예상 코드 리뷰 소요 시간🎯 2 (Simple) | ⏱️ ~15 분 관련 가능성 있는 PR
권장 리뷰어
축하 시
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/shared/components/storecard/StoreCardInMultiPinList.tsx (1)
95-124:⚠️ Potential issue | 🟠 Major초기 스크랩 상태가 아이콘에 반영되지 않을 수 있음
getMarkerIcon()이selectedStores만 기준으로 판단해서, 해당 세션에서 조작 이력이 없는 매장은scrap=true여도NotSave가 표시될 수 있습니다. 초기 렌더에서 저장 상태가 뒤집히는 UX 버그로 보입니다.selectedStores에 항목이 없을 때는isKeep(=scrap 반영)로 폴백해 주세요.✅ 제안 수정
const getMarkerIcon = () => { const groupIds = selectedStores[resId]; - if (!groupIds || groupIds.length === 0) { - return NotSave; - } + if (!groupIds) { + return isKeep ? Save : NotSave; + } + if (groupIds.length === 0) { + return NotSave; + } if (groupIds.length >= 2) { return MultiSaveMarker; } return Save; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/storecard/StoreCardInMultiPinList.tsx` around lines 95 - 124, getMarkerIcon currently decides the icon solely from selectedStores[resId], causing items with no session selection to show NotSave even when isKeep (scrap) is true; update getMarkerIcon to fall back to isKeep when groupIds is undefined or empty: if no groupIds, return Save when isKeep is true otherwise NotSave, keep the existing MultiSaveMarker logic for groupIds.length >= 2, and ensure the component references resId, selectedStores and isKeep as before.src/shared/components/restaurant_detail/DetailHeader.tsx (1)
114-144:⚠️ Potential issue | 🟠 Major서버 스크랩 상태가 아이콘에 반영되지 않을 수 있음
getMarkerIcon()이selectedStores만 보고 판단해, 아직 store에 반영되지 않은isScrap=true상태를notsave로 렌더링할 수 있습니다. 초기 진입 시 아이콘이 반대로 보일 수 있어 폴백이 필요합니다.✅ 제안 수정
const getMarkerIcon = () => { const groupIds = selectedStores[resId]; - if (!groupIds || groupIds.length === 0) { - return notsave; - } + if (!groupIds) { + return isKeep ? save : notsave; + } + if (groupIds.length === 0) { + return notsave; + } if (groupIds.length >= 2) { return MultiSaveMarker; } return save; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/restaurant_detail/DetailHeader.tsx` around lines 114 - 144, getMarkerIcon currently decides the marker solely from selectedStores[resId], which can miss a pending server scrap state (isScrap / isKeep) and render the wrong icon; update getMarkerIcon to use selectedStores[resId] if present but fall back to the local prop/state (isScrap or isKeep) when groupIds is empty/undefined so the UI reflects the most recent scrap state, and ensure the function still returns notsave, MultiSaveMarker, or save as before (refer to getMarkerIcon, selectedStores, resId, isScrap/isKeep).src/shared/components/storecard/StoreCardInSave.tsx (1)
46-103:⚠️ Potential issue | 🟠 Major초기 로딩 시 저장 아이콘이 잘못 표시될 수 있음
getMarkerIcon()이selectedStores만 기준이라 초기 로딩에서 데이터가 아직 없으면 저장 리스트임에도NotSave가 표시될 수 있습니다.selectedStores미존재 시isKeep폴백으로 처리하는 것이 안전합니다.✅ 제안 수정
const getMarkerIcon = () => { const groupIds = selectedStores[restaurantId]; - if (!groupIds || groupIds.length === 0) { - return NotSave; - } + if (!groupIds) { + return isKeep ? Save : NotSave; + } + if (groupIds.length === 0) { + return NotSave; + } if (groupIds.length >= 2) { return MultiSaveMarker; } return Save; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/storecard/StoreCardInSave.tsx` around lines 46 - 103, getMarkerIcon currently only checks selectedStores and returns NotSave when selectedStores[restaurantId] is missing, causing wrong icon on initial load; update getMarkerIcon to fall back to the isKeep state when selectedStores is undefined or has no entry for restaurantId (i.e., if !groupIds use isKeep ? Save : NotSave), keeping the existing multi-item and single-item checks (groupIds.length >= 2 -> MultiSaveMarker, else Save) and ensure getMarkerIcon reads the isKeep boolean so the initial render reflects the local saved state until selectedStores populates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/shared/components/restaurant_detail/DetailHeader.tsx`:
- Around line 114-144: getMarkerIcon currently decides the marker solely from
selectedStores[resId], which can miss a pending server scrap state (isScrap /
isKeep) and render the wrong icon; update getMarkerIcon to use
selectedStores[resId] if present but fall back to the local prop/state (isScrap
or isKeep) when groupIds is empty/undefined so the UI reflects the most recent
scrap state, and ensure the function still returns notsave, MultiSaveMarker, or
save as before (refer to getMarkerIcon, selectedStores, resId, isScrap/isKeep).
In `@src/shared/components/storecard/StoreCardInMultiPinList.tsx`:
- Around line 95-124: getMarkerIcon currently decides the icon solely from
selectedStores[resId], causing items with no session selection to show NotSave
even when isKeep (scrap) is true; update getMarkerIcon to fall back to isKeep
when groupIds is undefined or empty: if no groupIds, return Save when isKeep is
true otherwise NotSave, keep the existing MultiSaveMarker logic for
groupIds.length >= 2, and ensure the component references resId, selectedStores
and isKeep as before.
In `@src/shared/components/storecard/StoreCardInSave.tsx`:
- Around line 46-103: getMarkerIcon currently only checks selectedStores and
returns NotSave when selectedStores[restaurantId] is missing, causing wrong icon
on initial load; update getMarkerIcon to fall back to the isKeep state when
selectedStores is undefined or has no entry for restaurantId (i.e., if !groupIds
use isKeep ? Save : NotSave), keeping the existing multi-item and single-item
checks (groupIds.length >= 2 -> MultiSaveMarker, else Save) and ensure
getMarkerIcon reads the isKeep boolean so the initial render reflects the local
saved state until selectedStores populates.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/shared/assets/common/multiSaveMarker.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
src/shared/components/restaurant_detail/DetailHeader.tsxsrc/shared/components/storecard/StoreCardInMultiPinList.tsxsrc/shared/components/storecard/StoreCardInSave.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/shared/components/restaurant_detail/DetailHeader.tsx (1)
114-131:getMarkerIcon은 공용 유틸/훅으로 추출을 권장합니다.현재 동일 판정 로직이 카드/상세 컴포넌트에 반복되어 있어 이후 조건 변경 시 누락 위험이 큽니다. 공용화하면 유지보수성이 좋아집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/restaurant_detail/DetailHeader.tsx` around lines 114 - 131, The getMarkerIcon logic is duplicated across components; extract it into a shared utility or custom hook (e.g., useMarkerIcon or getMarkerIcon in a shared utils file) and replace local implementations in DetailHeader (getMarkerIcon) and the Card component to call the single shared function; ensure the shared function accepts the same inputs used here (resId, selectedStores, isKeep) and returns the same marker values (notsave, save, MultiSaveMarker) so existing callers keep behavior 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 `@src/shared/components/restaurant_detail/DetailHeader.tsx`:
- Around line 114-131: getMarkerIcon currently decides image source from
selectedStores/resId while elsewhere the alt text relies on isKeep, causing a
one-frame mismatch; unify the decision by extracting a single canonical
"isSaved" (or markerState) computed from selectedStores, resId, and isKeep
(reusing the existing logic in getMarkerIcon) and then derive both the icon
(MultiSaveMarker/save/notsave) and alt text from that single value; update
getMarkerIcon (or replace it with getMarkerState) to return a simple
enum/boolean and use that to pick the image and alt so src and alt are always
computed from the same criteria (referencing getMarkerIcon, selectedStores,
resId, isKeep, MultiSaveMarker, save, notsave).
In `@src/shared/components/storecard/StoreCardInMultiPinList.tsx`:
- Around line 95-112: getMarkerIcon() computes the icon to render but the image
alt text is using isKeep directly, causing a brief mismatch between src and alt
during state transitions; update the component to derive both the img src and
alt from the same computed value by calling getMarkerIcon() (or storing its
result in a local variable) and use that value for the image source and the alt
string, ensuring consistency for identifiers like getMarkerIcon, resId,
selectedStores, isKeep, NotSave, Save, and MultiSaveMarker.
---
Nitpick comments:
In `@src/shared/components/restaurant_detail/DetailHeader.tsx`:
- Around line 114-131: The getMarkerIcon logic is duplicated across components;
extract it into a shared utility or custom hook (e.g., useMarkerIcon or
getMarkerIcon in a shared utils file) and replace local implementations in
DetailHeader (getMarkerIcon) and the Card component to call the single shared
function; ensure the shared function accepts the same inputs used here (resId,
selectedStores, isKeep) and returns the same marker values (notsave, save,
MultiSaveMarker) so existing callers keep behavior unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/shared/components/restaurant_detail/DetailHeader.tsxsrc/shared/components/storecard/StoreCardInMultiPinList.tsxsrc/shared/components/storecard/StoreCardInSave.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/components/storecard/StoreCardInSave.tsx
Comment on lines
+114
to
+131
| const getMarkerIcon = () => { | ||
| const hasUserAction = resId in selectedStores; | ||
| const groupIds = selectedStores[resId]; | ||
|
|
||
| // 사용자가 조작을 한 기록이 있다면, 스토어의 데이터만 보고 판단합니다. | ||
| if (hasUserAction) { | ||
| if (!groupIds || groupIds.length === 0) { | ||
| return notsave; // 모든 그룹 해제 시 확실하게 notsave 반환 | ||
| } | ||
| if (groupIds.length >= 2) { | ||
| return MultiSaveMarker; | ||
| } | ||
| return save; | ||
| } | ||
|
|
||
| // 2. 조작 기록이 전혀 없는 초기 상태일 때만 부모의 scrap(isKeep) 상태를 따릅니다. | ||
| return isKeep ? save : notsave; | ||
| }; |
There was a problem hiding this comment.
아이콘 소스와 alt 판정 기준이 달라 접근성 상태가 일시적으로 어긋날 수 있습니다.
Line 148은 getMarkerIcon()(스토어 직접 기준), Line 149는 isKeep(상태 동기화 기준)를 사용해서, 상태 전환 직후 한 프레임 동안 src와 alt가 불일치할 수 있습니다. 같은 기준값으로 계산하는 게 안전합니다.
수정 예시
+ const markerIcon = getMarkerIcon();
+ const isSavedIcon = markerIcon !== notsave;
+
return (
<div className="relative flex w-full flex-col">
@@
<img
- src={getMarkerIcon()}
- alt={isKeep ? 'saved' : 'unsaved'}
+ src={markerIcon}
+ alt={isSavedIcon ? 'saved' : 'unsaved'}
className="h-[32px] w-[32px]"
/>Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/restaurant_detail/DetailHeader.tsx` around lines 114 -
131, getMarkerIcon currently decides image source from selectedStores/resId
while elsewhere the alt text relies on isKeep, causing a one-frame mismatch;
unify the decision by extracting a single canonical "isSaved" (or markerState)
computed from selectedStores, resId, and isKeep (reusing the existing logic in
getMarkerIcon) and then derive both the icon (MultiSaveMarker/save/notsave) and
alt text from that single value; update getMarkerIcon (or replace it with
getMarkerState) to return a simple enum/boolean and use that to pick the image
and alt so src and alt are always computed from the same criteria (referencing
getMarkerIcon, selectedStores, resId, isKeep, MultiSaveMarker, save, notsave).
Comment on lines
+95
to
+112
| const getMarkerIcon = () => { | ||
| const hasUserAction = resId in selectedStores; | ||
| const groupIds = selectedStores[resId]; | ||
|
|
||
| // 사용자가 조작을 한 기록이 있다면, 스토어의 데이터만 보고 판단합니다. | ||
| if (hasUserAction) { | ||
| if (!groupIds || groupIds.length === 0) { | ||
| return NotSave; // 모든 그룹 해제 시 확실하게 NotSave 반환 | ||
| } | ||
| if (groupIds.length >= 2) { | ||
| return MultiSaveMarker; | ||
| } | ||
| return Save; | ||
| } | ||
|
|
||
| // 2. 조작 기록이 전혀 없는 초기 상태일 때만 부모의 scrap(isKeep) 상태를 따릅니다. | ||
| return isKeep ? Save : NotSave; | ||
| }; |
There was a problem hiding this comment.
img src와 alt가 서로 다른 상태 소스를 사용합니다.
Line 128은 getMarkerIcon() 결과, Line 129는 isKeep를 사용해 전환 시점에 접근성 텍스트가 잠시 틀릴 수 있습니다. alt도 아이콘 계산 결과와 동일 기준으로 맞춰 주세요.
수정 예시
+ const markerIcon = getMarkerIcon();
+ const isSavedIcon = markerIcon !== NotSave;
@@
<img
- src={getMarkerIcon()}
- alt={isKeep ? 'saved' : 'unsaved'}
+ src={markerIcon}
+ alt={isSavedIcon ? 'saved' : 'unsaved'}
className="h-[25px] w-[25px]"
/>Also applies to: 127-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/storecard/StoreCardInMultiPinList.tsx` around lines 95
- 112, getMarkerIcon() computes the icon to render but the image alt text is
using isKeep directly, causing a brief mismatch between src and alt during state
transitions; update the component to derive both the img src and alt from the
same computed value by calling getMarkerIcon() (or storing its result in a local
variable) and use that value for the image source and the alt string, ensuring
consistency for identifiers like getMarkerIcon, resId, selectedStores, isKeep,
NotSave, Save, and MultiSaveMarker.
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
2 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.
📝 작업 내용
📢 PR Point
이미지 첨부
🔧 다음 할 일
Summary by CodeRabbit
릴리스 노트