[NDGL-100] 장소 상세보기 관련 UI/UX 수정#29
Merged
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughPlaceInfo 데이터 클래스를 공유 모델로 중앙화하고 id를 googlePlaceId로 변경합니다. AddItinerary, AddPlace, FollowTravel, PlaceDetail 등 여러 기능에서 PlaceInfo를 사용하도록 업데이트하며, 네비게이션 파라미터를 placeId에서 googlePlaceId로 변경합니다. UI 컴포넌트를 공유 버전으로 통합하고 TravelPlace 데이터 모델을 리팩토링합니다. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 이 PR은 매우 광범위한 리팩토링으로 여러 기능에 걸쳐 PlaceInfo 모델을 중앙화하고, 데이터 구조를 재설계하며, 네비게이션 파라미터를 변경합니다. 26개 이상의 파일이 영향을 받으며, 각 변경은 데이터 접근 패턴, UI 컴포넌트 구성, 네비게이션 플로우 변경을 포함합니다. 복합적인 리팩토링으로 인해 자세한 검토와 통합 테스트가 필요합니다. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt (1)
52-52: 🛠️ Refactor suggestion | 🟠 Major
AddPlace.placeId를googlePlaceId로 변경하세요
PlaceDetail,FollowPlaceDetail은googlePlaceId로 변경되었으나AddPlace는 여전히placeId를 사용합니다. 네이밍 일관성을 위해 변경이 필요합니다.♻️ 변경 사항
- data class AddPlace(val placeId: String) : Route + data class AddPlace(val googlePlaceId: String) : Route
TravelEntry.kt(line 156):- factory.create(placeId = route.placeId) + factory.create(placeId = route.googlePlaceId)
AddPlaceViewModel.kt(constructor parameter 이름은 유지 가능):-class AddPlaceViewModel `@AssistedInject` constructor( - `@Assisted` private val placeId: String, +class AddPlaceViewModel `@AssistedInject` constructor( + `@Assisted` private val googlePlaceId: String,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt` at line 52, AddPlace 데이터클래스의 프로퍼티명 placeId를 googlePlaceId로 변경하고, 이 변경에 따라 AddPlace를 생성하거나 매칭하는 모든 호출부와 패턴(예: when, nav args, serialization/deserialization), TravelEntry에서의 사용 부분 그리고 관련 네비게이션 생성 지점들을 함께 업데이트하세요; AddPlaceViewModel의 생성자 파라미터 이름은 유지해도 되지만 내부에서 참조하는 필드명도 googlePlaceId로 맞추어 오류가 생기지 않도록 확인하세요.
🧹 Nitpick comments (13)
feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/component/SearchedPlaceBottomSheet.kt (1)
162-165:navBarSectionHeight변수명이 실제 역할과 맞지 않습니다.이 48dp 값은
PlaceDetailTabRow(탭 바)의 높이를 의미하는데, 변수명navBarSectionHeight는NDGLNavigationBar로 오해하기 쉽습니다.NDGLNavigationBar는 스크롤 영역 바깥(상단 Box)에 위치하므로 여기서의 48dp는 탭 바와 관계있습니다.♻️ 변수명 개선 제안
-val navBarSectionHeight = 48.dp -val thumbnailHeightPx = with(density) { thumbnailHeight.toPx() } -val navBarSectionHeightPx = with(density) { navBarSectionHeight.toPx() } -val maxCollapseHeightPx = navBarSectionHeightPx + thumbnailHeightPx +val tabRowHeight = 48.dp +val thumbnailHeightPx = with(density) { thumbnailHeight.toPx() } +val tabRowHeightPx = with(density) { tabRowHeight.toPx() } +val maxCollapseHeightPx = tabRowHeightPx + thumbnailHeightPx
thumbnailProgress계산식도 동일하게 변경 필요:-val thumbnailProgress = (1f - (collapseOffset - navBarSectionHeightPx).coerceAtLeast(0f) / thumbnailHeightPx).coerceIn(0f, 1f) +val thumbnailProgress = (1f - (collapseOffset - tabRowHeightPx).coerceAtLeast(0f) / thumbnailHeightPx).coerceIn(0f, 1f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/component/SearchedPlaceBottomSheet.kt` around lines 162 - 165, 현재 navBarSectionHeight라는 이름이 실제로는 PlaceDetailTabRow(탭 바)의 높이(48.dp)를 가리켜 오해의 소지가 있습니다; navBarSectionHeight를 placeDetailTabRowHeight 또는 tabBarHeight 같은 명확한 이름으로 변경하고 thumbnailProgress를 계산하는 식(참조: thumbnailHeight, thumbnailHeightPx, maxCollapseHeightPx 등)이 새 이름을 사용하도록 모두 업데이트하세요; 또한 NDGLNavigationBar와 혼동되지 않도록 관련 주석도 갱신해 주세요.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/component/PlaceItem.kt (1)
220-298: 세 프리뷰의PlaceInfo생성 코드 중복 — 공유 상수 추출 권장
PlaceItemPreview,EditablePlaceItemUncheckedPreview,EditablePlaceItemCheckedPreview가 동일한PlaceInfo및TravelPlace.UserData인스턴스를 반복 생성합니다. 프리뷰 전용 상수 하나로 추출하면 유지보수가 용이해집니다.♻️ 리팩토링 제안
+private val previewTravelPlace = TravelPlace( + id = 1, + placeInfo = PlaceInfo( + day = 1, + sequence = 1, + googlePlaceId = "", + thumbnail = "", + latitude = 35.6585805, + longitude = 139.7454329, + name = "도쿄 타워", + googleMapsUri = "", + placeType = PlaceType.ATTRACTION, + ), + userData = TravelPlace.UserData(estimatedDuration = 60.minutes), + startTime = 10.hours, +) `@Preview` `@Composable` private fun PlaceItemPreview() { NDGLTheme { PlaceItem( - place = TravelPlace( - id = 1, - placeInfo = PlaceInfo( - day = 1, - sequence = 1, - googlePlaceId = "", - thumbnail = "", - latitude = 35.6585805, - longitude = 139.7454329, - name = "도쿄 타워", - googleMapsUri = "", - placeType = PlaceType.ATTRACTION, - ), - userData = TravelPlace.UserData(estimatedDuration = 60.minutes), - startTime = 10.hours, - ), + place = previewTravelPlace, onClick = {}, onLongClick = {}, ) } }
EditablePlaceItemUncheckedPreview/EditablePlaceItemCheckedPreview도 동일하게previewTravelPlace를 재사용하되,startTime이 다른 경우copy(startTime = 12.hours)를 활용합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/component/PlaceItem.kt` around lines 220 - 298, Extract a shared preview TravelPlace instance (e.g., previewTravelPlace) containing the common PlaceInfo and TravelPlace.UserData used in PlaceItemPreview, EditablePlaceItemUncheckedPreview, and EditablePlaceItemCheckedPreview, then reuse it in each preview and adjust differing fields with copy(startTime = 12.hours) where needed; update PlaceItemPreview, EditablePlaceItemUncheckedPreview, and EditablePlaceItemCheckedPreview to reference previewTravelPlace (and call copy(...) to change startTime for the editable previews) to remove duplication while keeping existing onClick/onLongClick/onCheck parameters.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlacePhotoTab.kt (1)
23-23: 불필요한kotlin.collections.forEachimport 제거 권장
forEach는 Kotlin 표준 라이브러리에서 자동으로 사용 가능한 확장 함수로, 명시적 import가 필요 없습니다.♻️ 제안된 수정
-import kotlin.collections.forEach🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlacePhotoTab.kt` at line 23, Remove the unnecessary explicit import of kotlin.collections.forEach in PlacePhotoTab (the import line "import kotlin.collections.forEach"); forEach is a Kotlin stdlib extension and does not require an explicit import, so delete that import statement to clean up unused imports in the PlacePhotoTab file/class.navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt (1)
29-40:alternativePlaces기본값 타입 불일치에 대한 설계 의도 명시 필요
PlaceDetail.alternativePlaces는List<RouteAlternativePlace>? = null이고,FollowPlaceDetail.alternativePlaces는List<RouteAlternativePlace> = emptyList()로 정의되어 있습니다. 현재 코드는 TravelEntry.kt에서 이 차이를 명시적으로 처리하고 있습니다 (FollowPlaceDetail 생성 시 null을 emptyList()로 변환). 이러한 설계 선택이 의도적이라면, 각 타입에 주석을 추가하여 왜 다른 기본값을 사용하는지 명시하는 것이 좋습니다. PlaceDetail은 개인 여행용(nullable 허용), FollowPlaceDetail은 팔로우 여행용(항상 비어있는 리스트)이라는 의도를 코드 레벨에서 명확히 하면 향후 유지보수가 용이해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt` around lines 29 - 40, Add clear KDoc comments above the data classes PlaceDetail and FollowPlaceDetail explaining the design intent for alternativePlaces: state that PlaceDetail.alternativePlaces is nullable (List<RouteAlternativePlace>? = null) to represent optional personal-travel alternatives, while FollowPlaceDetail.alternativePlaces uses a non-null empty list (List<RouteAlternativePlace> = emptyList()) because follow-trips always normalize null to empty and consumers expect a non-null collection; also reference in the comment that TravelEntry.kt explicitly converts null to emptyList() when constructing FollowPlaceDetail so readers know where the normalization occurs.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/AddItineraryContract.kt (1)
85-92:placeId→googlePlaceId이름 변경 누락
SelectablePlace,SearchResult,CheckSelectablePlace,ClickSelectablePlace등 AddItinerary의 대부분의 클래스와PlaceInfo모델에서는googlePlaceId를 사용하지만,BookMarkPlace와NavigateToAddPlace는 여전히placeId를 사용하고 있어 일관성이 떨어집니다. 특히 AddItineraryViewModel.kt:271에서googlePlaceId를NavigateToAddPlace(placeId)로 전달하고 있으므로, 의미론적 일관성을 위해 이 두 클래스의 파라미터명을googlePlaceId로 변경하는 것을 권장합니다.♻️ 제안된 수정
- data class BookMarkPlace(val placeId: String) : AddItineraryIntent + data class BookMarkPlace(val googlePlaceId: String) : AddItineraryIntent - data class NavigateToAddPlace(val placeId: String) : AddItinerarySideEffect + data class NavigateToAddPlace(val googlePlaceId: String) : AddItinerarySideEffect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/AddItineraryContract.kt` around lines 85 - 92, The parameter name inconsistency: change the BookMarkPlace data class and the NavigateToAddPlace side-effect to use googlePlaceId instead of placeId (i.e., BookMarkPlace(val googlePlaceId: String) and NavigateToAddPlace(val googlePlaceId: String)), and update all call sites (for example the place where NavigateToAddPlace(placeId) is constructed and any BookMarkPlace(...) usages) to pass and reference googlePlaceId so names are consistent with SelectablePlace, SearchResult, CheckSelectablePlace, ClickSelectablePlace and PlaceInfo.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/TravelDetailContract.kt (1)
105-105: Intent와 SideEffect 간 파라미터 네이밍 불일치
TravelDetailIntent.NavigateToTravelPlaceDetail은placeId: String(Line 105)을 사용하지만,TravelDetailSideEffect.NavigateToTravelPlaceDetail은googlePlaceId: String(Line 117)을 사용합니다. 이 PR에서placeId→googlePlaceId로 전환하고 있으므로, Intent 쪽도googlePlaceId로 통일하는 것이 일관성 있습니다.♻️ 네이밍 통일 제안
- data class NavigateToTravelPlaceDetail(val placeId: String) : TravelDetailIntent + data class NavigateToTravelPlaceDetail(val googlePlaceId: String) : TravelDetailIntentAlso applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/TravelDetailContract.kt` at line 105, Update the parameter name in TravelDetailIntent.NavigateToTravelPlaceDetail from placeId: String to googlePlaceId: String so it matches TravelDetailSideEffect.NavigateToTravelPlaceDetail; update all usages/constructor calls and any pattern matching or when branches that reference NavigateToTravelPlaceDetail to use googlePlaceId to keep naming consistent across intent and side-effect handling.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/FollowTravelContract.kt (1)
38-44:NavigateToFollowPlaceDetail의 파라미터명이placeId로 남아있습니다.
TravelDetailSideEffect.NavigateToTravelPlaceDetail에서는googlePlaceId로 변경되었지만, 이 파일의NavigateToFollowPlaceDetail은 여전히placeId를 사용합니다. 일관성을 위해googlePlaceId로 통일하는 것을 권장합니다.♻️ 네이밍 통일 제안
sealed interface FollowTravelSideEffect : UiSideEffect { data class NavigateToFollowPlaceDetail( - val placeId: String, + val googlePlaceId: String, val tipContent: TipContent?, val alternativePlaces: List<AlternativePlace>?, ) : FollowTravelSideEffect }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/FollowTravelContract.kt` around lines 38 - 44, Rename the NavigateToFollowPlaceDetail data class parameter from placeId to googlePlaceId in FollowTravelSideEffect (key symbol: NavigateToFollowPlaceDetail) and update all usages/signatures accordingly (references in constructors, callers, and any mapping to TipContent or AlternativePlace). Ensure the property name change is reflected in serialization/parcelization and in any code that constructs or deconstructs NavigateToFollowPlaceDetail so the type remains consistent with TravelDetailSideEffect.NavigateToTravelPlaceDetail.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/addplace/AddPlaceViewModel.kt (1)
19-19: AddPlaceViewModel의 파라미터명을googlePlaceId로 통일 필요PlaceDetailViewModel과 FollowPlaceDetailViewModel은 이미
googlePlaceId로 통일되어 있으나, AddPlaceViewModel만placeId를 사용 중입니다. 코드베이스 전체의 일관성을 위해 여기도 함께 변경하는 것을 권장합니다.변경 대상:
- 생성자 파라미터 (19행)
- Factory 메서드 시그니처 (92-95행)
- 내부 사용처:
loadPlaceDetail()(30행),loadPlacePhotos()(44행)- 호출 지점: TravelEntry.kt의
factory.create(placeId = ...)도 함께 변경 필요🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/addplace/AddPlaceViewModel.kt` at line 19, AddPlaceViewModel의 생성자 파라미터명 placeId를 googlePlaceId로 바꿔 일관성 유지하세요: 생성자 선언(현재 `@Assisted` private val placeId)과 Factory 메서드 시그니처를 googlePlaceId로 변경하고, 내부에서 참조되는 loadPlaceDetail()와 loadPlacePhotos() 호출부도 모두 새 이름으로 전달되도록 수정하며, 이 뷰모델을 생성하는 호출(예: TravelEntry.kt의 factory.create(placeId = ...))도 factory.create(googlePlaceId = ...)로 함께 업데이트하세요.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/model/TipContent.kt (1)
3-6:tips를 nullable로 변경 +PlaceTipCard의require()조합 — 런타임 크래시 위험
tips: List<String>? = null로 변경하면서PlaceTipCard내부에서require(tips != null)로 null 여부를 강제 검증하는 패턴이 됩니다. Composable이 예외를 던질 경우 어떻게 전파될지 명확히 정의되어 있지 않으며, 현재 호출부인PlaceInfoTab은isNullOrEmpty()가드로 보호하고 있지만, 다른 곳에서PlaceTipCard를 직접 호출하면IllegalArgumentException으로 앱이 크래시됩니다.API 계약을 컴파일 타임에 보장하려면
tips를 non-nullable로 유지하거나,PlaceTipCard내부에서 null/empty 조건을 graceful하게 처리하는 것을 권장합니다.♻️ 대안: `tips`를 non-nullable로 유지
data class TipContent( val creatorName: String = "", - val tips: List<String>? = null, + val tips: List<String> = emptyList(), )그에 맞춰
PlaceTipCard.kt도 수정:- require(tips != null) - val pagerState = rememberPagerState(pageCount = { tips.size }) + if (tips.isEmpty()) return + val pagerState = rememberPagerState(pageCount = { tips.size })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/model/TipContent.kt` around lines 3 - 6, The TipContent.tips field was made nullable which forces callers like PlaceTipCard to use require(tips != null) and risks runtime IllegalArgumentException; revert tips to a non-nullable list (e.g., change TipContent.tips to val tips: List<String> = emptyList()) and update PlaceTipCard to stop using require(...) and instead work with the non-null list (or alternatively, if you prefer nullable, remove the require and handle null/empty gracefully inside PlaceTipCard). Update any callers (e.g., PlaceInfoTab) to pass/expect a non-null list and remove redundant null checks like isNullOrEmpty() where appropriate so the API contract is enforced at compile time.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/TravelMap.kt (1)
122-124:key {}블록 누락 —traveldetail버전과 불일치
traveldetail/component/TravelMap.kt은key(place.id, place.placeInfo.sequence) { PlaceMarker(...) }블록을 사용하지만, 이 파일의forEach에는key {}블록이 없습니다. 리스트가 재정렬되거나 중간 항목이 삭제될 경우 Compose가 위치 기반으로 composition slot을 재사용하려 할 수 있어 비효율적인 recomposition이 발생할 수 있습니다.♻️ key 블록 추가 제안
+import androidx.compose.runtime.key places.forEach { place -> - PlaceMarker(place = place) + key(place.id, place.placeInfo.sequence) { + PlaceMarker(place = place) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/TravelMap.kt` around lines 122 - 124, The places.forEach loop in TravelMap.kt lacks a key block which can cause wrong composition reuse; wrap the PlaceMarker composable inside a stable key using the place identifiers (e.g., place.id and place.placeInfo.sequence) so Compose can track items across reorders/deletions; locate the loop that calls PlaceMarker(place = place) and replace it with a key(...) { PlaceMarker(...) } using those unique symbols to prevent position-based slot reuse.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlaceTipCard.kt (1)
39-43: Composable 내부require()— null일 경우 앱 크래시
require(tips != null)이 실패하면IllegalArgumentException이 발생하며, Composable이 예외를 던질 때의 동작이 명확히 정의되어 있지 않습니다. 현재는PlaceInfoTab이isNullOrEmpty()가드로 보호하고 있지만, 이 컴포넌트를 다른 곳에서 재사용할 경우 크래시 위험이 있습니다.
TipContent.tips를 non-nullable로 변경하거나(TipContent.kt 코멘트 참고), null/empty 시 gracefully 렌더링하지 않는 방식으로 수정하는 것을 권장합니다.♻️ 제안된 수정 (null guard early return)
`@Composable` internal fun PlaceTipCard(tipContent: TipContent) { val creatorName = tipContent.creatorName val tips = tipContent.tips - require(tips != null) + if (tips.isNullOrEmpty()) return val pagerState = rememberPagerState(pageCount = { tips.size })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlaceTipCard.kt` around lines 39 - 43, Replace the unsafe require(tips != null) in the PlaceTipCard composable with a graceful null/empty guard: check TipContent.tips via if (tips.isNullOrEmpty()) return (or render an empty placeholder) so the composable exits early instead of throwing; then only call rememberPagerState(pageCount = { tips.size }) after the guard so pagerState never receives a null/zero source. Update references in PlaceTipCard (tips, pagerState, rememberPagerState) to rely on the early-return guard; alternatively consider making TipContent.tips non-nullable in TipContent if you prefer model-level enforcement.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/PlaceItem.kt (1)
90-91:PlaceCard의 가시성이public으로 설정됨
PlaceItem은internal이고PlaceNumber는private인 반면,PlaceCard만public으로 선언되어 있습니다. 모듈 외부에서 사용하지 않는다면internal로 변경하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/PlaceItem.kt` around lines 90 - 91, PlaceCard is declared public while PlaceItem is internal and PlaceNumber is private; make PlaceCard internal to match module visibility and avoid exposing it outside the module. Change the function declaration of PlaceCard to internal (i.e., update the modifier on fun PlaceCard) and run a quick project-wide search for usages of PlaceCard to confirm no external modules depend on it; if any external use exists, either keep it public with justification or refactor callers into the same module.feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/placedetail/FollowPlaceDetailScreen.kt (1)
89-274:FollowPlaceDetailScreen과PlaceDetailScreen의 레이아웃 코드 중복두 화면의 레이아웃 구조(collapsible header, nested scroll, thumbnail, tab row, lazy column)가 거의 동일합니다.
PlaceDetailScreen과의 차이점은 모달/변경 기능 유무 정도입니다. 이번 PR에서 컴포넌트 통합이 잘 진행되고 있으므로, 추후 공통 레이아웃을 별도 컴포저블로 추출하면 유지보수성이 더 향상될 것입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/placedetail/FollowPlaceDetailScreen.kt` around lines 89 - 274, The FollowPlaceDetailScreen duplicates most of the collapsible header + nested scroll layout from PlaceDetailScreen; extract the common structure (nestedScrollConnection, collapseOffset logic, navBarProgress/thumbnailProgress calculations, thumbnail rendering, PlaceDetailTabRow and LazyColumn container) into a shared composable (e.g., CollapsiblePlaceDetailLayout or PlaceDetailLayout) that accepts parameters/callbacks for placeInfo, selectedTab, clickBackButton, selectTab, clickAddress, clickMenu, clickAlternativePlace and slot lambdas for the tab-specific content (INFO vs PHOTO) and optional modal/modify UI; then refactor FollowPlaceDetailScreen and PlaceDetailScreen to call that shared composable and pass only their differing behavior (modal flags, extra actions, and the tab content) so layout logic lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/component/SearchedPlaceBottomSheet.kt`:
- Around line 219-220: PlaceDetailTabRow already renders a HorizontalDivider
internally, so remove the extra HorizontalDivider call in
SearchedPlaceBottomSheet (the line immediately after
PlaceDetailTabRow(selectedTab = selectedTab, onTabSelected = { selectedTab = it
})) to avoid double dividers; keep only the PlaceDetailTabRow invocation (or, if
you prefer the external divider, remove the divider inside PlaceDetailTabRow) so
only one divider is rendered.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/AlternativePlaceContent.kt`:
- Line 50: AlternativePlace.id is hardcoded to an empty string when creating
AlternativePlace in FollowTravelViewModel (where AlternativePlace is constructed
with id = "") which causes onClick(alternativePlace.id) in
AlternativePlaceContent to pass an empty googlePlaceId; update the model/mapping
so the alternative place has the real Google Place ID—either add a googlePlaceId
field to PlanBPlace (and populate it when creating AlternativePlace) or change
the mapping logic in FollowTravelViewModel (the AlternativePlace creation code)
to supply the actual ID instead of "". Ensure AlternativePlaceContent continues
to call onClick(alternativePlace.id) and
FollowTravelViewModel/FollowPlaceDetailIntent receives the real ID.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlaceInfoTab.kt`:
- Around line 99-111: The divider currently shows when placeInfo.tipContent !=
null || placeInfo.alternativePlaces != null which can render a divider without
any content; change the divider condition to the same checks used for rendering
content so it only shows when there is actual content: use the same null/empty
checks as the PlaceTipCard and the alternative-places rendering (e.g.,
!placeInfo.tipContent?.tips.isNullOrEmpty() ||
!placeInfo.alternativePlaces.isNullOrEmpty()), updating the condition near the
divider and keeping PlaceTipCard(tipContent = placeInfo.tipContent) and the
alternative places block unchanged.
- Around line 108-111: placeInfo.tipContent is nullable but PlaceTipCard expects
a non-null TipContent; change the null-check logic so you first obtain a
non-null TipContent reference and only then call PlaceTipCard. For example,
guard using a local val or use Kotlin's let/takeIf to ensure tipContent is
non-null and its tips list is not empty, then call PlaceTipCard with that
non-null TipContent (referencing placeInfo.tipContent, TipContent, tips, and
PlaceTipCard).
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/FollowTravelViewModel.kt`:
- Around line 113-116: The code has a race: toItinerary() reads
state.value.contentInfo.videoInfo.creatorName while
loadTravelTemplateItinerary() and loadTravelTemplateContentInfo() run
concurrently, causing creatorName to be empty if itinerary arrives first; fix by
making contentInfo load before mapping or by deferring injection of
creatorName—either sequence loadTravelTemplateContentInfo() and on its success
call loadTravelTemplateItinerary(), or change toItinerary()/TipContent
construction to omit reading state.value.contentInfo.videoInfo.creatorName
(leave creatorName empty or null) and then, when loadTravelTemplateContentInfo()
completes, update the mapped itinerary/TipContent with the actual creatorName.
- Around line 117-124: The mapping for alternativePlaces is incorrectly using
the parent item's category (item.place.category.toPlaceType()) because
PlanBPlace currently lacks a category; update the PlanBPlace data model to
include a category field, populate it where PlanBPlace instances are created,
and change the mapper in FollowTravelViewModel (the item.planB.map block that
constructs AlternativePlace) to use planB.category.toPlaceType() for placeType
(keep other fields like name and feature as-is and set id appropriately if
needed).
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/model/ContentInfo.kt`:
- Line 4: The ContentInfo data model declares travelId as Int which is
inconsistent with the rest of the codebase and the server model
(TravelTemplateContentInfo.travelId: Long) and can lead to overflow/data loss;
change the travelId property in ContentInfo to Long (use 0L as the default) and
then update any constructors, mappers or usages that instantiate ContentInfo
(e.g., mapping functions from TravelTemplateContentInfo, places referencing
ContentInfo.travelId) to accept/propagate Longs so the type is uniform across
Route/ViewModel/Contract and matches the server model.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/navigation/TravelEntry.kt`:
- Around line 56-60: The nested null-safe lets around tipContent drop
creatorName when content.tips is null; update the mapping that builds
RouteTipContent so it preserves creatorName and supplies a non-null tips
fallback (e.g., use content.tips ?: emptyList()) like: tipContent?.let { content
-> RouteTipContent(creatorName = content.creatorName, tips = content.tips ?:
emptyList()) }, and apply the same fix to the other occurrence; if the original
intent was to allow null result when tips is null, instead add a clarifying
comment near the tipContent -> RouteTipContent mapping explaining that behavior.
---
Outside diff comments:
In `@navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt`:
- Line 52: AddPlace 데이터클래스의 프로퍼티명 placeId를 googlePlaceId로 변경하고, 이 변경에 따라
AddPlace를 생성하거나 매칭하는 모든 호출부와 패턴(예: when, nav args,
serialization/deserialization), TravelEntry에서의 사용 부분 그리고 관련 네비게이션 생성 지점들을 함께
업데이트하세요; AddPlaceViewModel의 생성자 파라미터 이름은 유지해도 되지만 내부에서 참조하는 필드명도 googlePlaceId로
맞추어 오류가 생기지 않도록 확인하세요.
---
Nitpick comments:
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/AddItineraryContract.kt`:
- Around line 85-92: The parameter name inconsistency: change the BookMarkPlace
data class and the NavigateToAddPlace side-effect to use googlePlaceId instead
of placeId (i.e., BookMarkPlace(val googlePlaceId: String) and
NavigateToAddPlace(val googlePlaceId: String)), and update all call sites (for
example the place where NavigateToAddPlace(placeId) is constructed and any
BookMarkPlace(...) usages) to pass and reference googlePlaceId so names are
consistent with SelectablePlace, SearchResult, CheckSelectablePlace,
ClickSelectablePlace and PlaceInfo.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/additinerary/component/SearchedPlaceBottomSheet.kt`:
- Around line 162-165: 현재 navBarSectionHeight라는 이름이 실제로는 PlaceDetailTabRow(탭 바)의
높이(48.dp)를 가리켜 오해의 소지가 있습니다; navBarSectionHeight를 placeDetailTabRowHeight 또는
tabBarHeight 같은 명확한 이름으로 변경하고 thumbnailProgress를 계산하는 식(참조: thumbnailHeight,
thumbnailHeightPx, maxCollapseHeightPx 등)이 새 이름을 사용하도록 모두 업데이트하세요; 또한
NDGLNavigationBar와 혼동되지 않도록 관련 주석도 갱신해 주세요.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/addplace/AddPlaceViewModel.kt`:
- Line 19: AddPlaceViewModel의 생성자 파라미터명 placeId를 googlePlaceId로 바꿔 일관성 유지하세요:
생성자 선언(현재 `@Assisted` private val placeId)과 Factory 메서드 시그니처를 googlePlaceId로 변경하고,
내부에서 참조되는 loadPlaceDetail()와 loadPlacePhotos() 호출부도 모두 새 이름으로 전달되도록 수정하며, 이 뷰모델을
생성하는 호출(예: TravelEntry.kt의 factory.create(placeId = ...))도
factory.create(googlePlaceId = ...)로 함께 업데이트하세요.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlacePhotoTab.kt`:
- Line 23: Remove the unnecessary explicit import of kotlin.collections.forEach
in PlacePhotoTab (the import line "import kotlin.collections.forEach"); forEach
is a Kotlin stdlib extension and does not require an explicit import, so delete
that import statement to clean up unused imports in the PlacePhotoTab
file/class.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/component/PlaceTipCard.kt`:
- Around line 39-43: Replace the unsafe require(tips != null) in the
PlaceTipCard composable with a graceful null/empty guard: check TipContent.tips
via if (tips.isNullOrEmpty()) return (or render an empty placeholder) so the
composable exits early instead of throwing; then only call
rememberPagerState(pageCount = { tips.size }) after the guard so pagerState
never receives a null/zero source. Update references in PlaceTipCard (tips,
pagerState, rememberPagerState) to rely on the early-return guard; alternatively
consider making TipContent.tips non-nullable in TipContent if you prefer
model-level enforcement.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/PlaceItem.kt`:
- Around line 90-91: PlaceCard is declared public while PlaceItem is internal
and PlaceNumber is private; make PlaceCard internal to match module visibility
and avoid exposing it outside the module. Change the function declaration of
PlaceCard to internal (i.e., update the modifier on fun PlaceCard) and run a
quick project-wide search for usages of PlaceCard to confirm no external modules
depend on it; if any external use exists, either keep it public with
justification or refactor callers into the same module.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/component/TravelMap.kt`:
- Around line 122-124: The places.forEach loop in TravelMap.kt lacks a key block
which can cause wrong composition reuse; wrap the PlaceMarker composable inside
a stable key using the place identifiers (e.g., place.id and
place.placeInfo.sequence) so Compose can track items across reorders/deletions;
locate the loop that calls PlaceMarker(place = place) and replace it with a
key(...) { PlaceMarker(...) } using those unique symbols to prevent
position-based slot reuse.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/FollowTravelContract.kt`:
- Around line 38-44: Rename the NavigateToFollowPlaceDetail data class parameter
from placeId to googlePlaceId in FollowTravelSideEffect (key symbol:
NavigateToFollowPlaceDetail) and update all usages/signatures accordingly
(references in constructors, callers, and any mapping to TipContent or
AlternativePlace). Ensure the property name change is reflected in
serialization/parcelization and in any code that constructs or deconstructs
NavigateToFollowPlaceDetail so the type remains consistent with
TravelDetailSideEffect.NavigateToTravelPlaceDetail.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/followtravel/placedetail/FollowPlaceDetailScreen.kt`:
- Around line 89-274: The FollowPlaceDetailScreen duplicates most of the
collapsible header + nested scroll layout from PlaceDetailScreen; extract the
common structure (nestedScrollConnection, collapseOffset logic,
navBarProgress/thumbnailProgress calculations, thumbnail rendering,
PlaceDetailTabRow and LazyColumn container) into a shared composable (e.g.,
CollapsiblePlaceDetailLayout or PlaceDetailLayout) that accepts
parameters/callbacks for placeInfo, selectedTab, clickBackButton, selectTab,
clickAddress, clickMenu, clickAlternativePlace and slot lambdas for the
tab-specific content (INFO vs PHOTO) and optional modal/modify UI; then refactor
FollowPlaceDetailScreen and PlaceDetailScreen to call that shared composable and
pass only their differing behavior (modal flags, extra actions, and the tab
content) so layout logic lives in one place.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/model/TipContent.kt`:
- Around line 3-6: The TipContent.tips field was made nullable which forces
callers like PlaceTipCard to use require(tips != null) and risks runtime
IllegalArgumentException; revert tips to a non-nullable list (e.g., change
TipContent.tips to val tips: List<String> = emptyList()) and update PlaceTipCard
to stop using require(...) and instead work with the non-null list (or
alternatively, if you prefer nullable, remove the require and handle null/empty
gracefully inside PlaceTipCard). Update any callers (e.g., PlaceInfoTab) to
pass/expect a non-null list and remove redundant null checks like
isNullOrEmpty() where appropriate so the API contract is enforced at compile
time.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/component/PlaceItem.kt`:
- Around line 220-298: Extract a shared preview TravelPlace instance (e.g.,
previewTravelPlace) containing the common PlaceInfo and TravelPlace.UserData
used in PlaceItemPreview, EditablePlaceItemUncheckedPreview, and
EditablePlaceItemCheckedPreview, then reuse it in each preview and adjust
differing fields with copy(startTime = 12.hours) where needed; update
PlaceItemPreview, EditablePlaceItemUncheckedPreview, and
EditablePlaceItemCheckedPreview to reference previewTravelPlace (and call
copy(...) to change startTime for the editable previews) to remove duplication
while keeping existing onClick/onLongClick/onCheck parameters.
In
`@feature/travel/src/main/java/com/yapp/ndgl/feature/travel/traveldetail/TravelDetailContract.kt`:
- Line 105: Update the parameter name in
TravelDetailIntent.NavigateToTravelPlaceDetail from placeId: String to
googlePlaceId: String so it matches
TravelDetailSideEffect.NavigateToTravelPlaceDetail; update all
usages/constructor calls and any pattern matching or when branches that
reference NavigateToTravelPlaceDetail to use googlePlaceId to keep naming
consistent across intent and side-effect handling.
In `@navigation/src/main/java/com/yapp/ndgl/navigation/Route.kt`:
- Around line 29-40: Add clear KDoc comments above the data classes PlaceDetail
and FollowPlaceDetail explaining the design intent for alternativePlaces: state
that PlaceDetail.alternativePlaces is nullable (List<RouteAlternativePlace>? =
null) to represent optional personal-travel alternatives, while
FollowPlaceDetail.alternativePlaces uses a non-null empty list
(List<RouteAlternativePlace> = emptyList()) because follow-trips always
normalize null to empty and consumers expect a non-null collection; also
reference in the comment that TravelEntry.kt explicitly converts null to
emptyList() when constructing FollowPlaceDetail so readers know where the
normalization occurs.
…lTabRow, PlaceInfoTab, PlacePhotoTab, PlaceTipCard
mj010504
force-pushed
the
feature/NDGL-100
branch
from
4dcd61c to
21230c3
Compare
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
1 participant
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 by CodeRabbit
릴리스 노트
새로운 기능
개선 사항