[Feat] PlaceMap의 하위 Fragment 제거 및 MVI 아키텍쳐 적용

[oungsi2000]

#️⃣ 이슈 번호

#18

🛠️ 작업 내용

1. PlaceMap 패키지의 하위 Fragment -> Screen으로 리팩토링
2. MVI 아키텍쳐 적용
3. ViewModel의 Delegate pattern 적용 (Handler)
4. PlaceMap 패키지 전용 스타일 추가

🙇🏻 중점 리뷰 요청

전체 아키텍쳐

MVI 특징

Composable의 파라미터는 크게 3가지로 나눌 수 있습니다.
주로 on~으로 시작하는 사용자 행동 (Action)
컴포저블의 상태 (UiState)
사이드 이펙트 (Event)

기존 MVVM에서는 컴포저블의 상태를 추상화 할 수 있었습니다. (산발된 LiveData를 하나의 UiState로 관리)
하지만 사이드 사용자 행동이 많아지면 그만큼 ViewModel의 메서드가 늘어나는 문제가 있었고,
사이드 이펙트 또한 추상화가 어려운 문제가 있었습니다.

(정확히는 UiState처럼 하나의 상태로 통합 관리하게 된다면
Composable이 ViewModel의 메서드를 직접 호출하기 때문에
Composable - ViewModel method의 결합이 높아지는 문제가 있습니다)

하지만 MVI의 경우 Composable은 어떤 액션 이 일어났는지 보고하면
ViewModel 내부에서 상태를 변경, 혹은 사이드 이펙트를 발생시키는 방식으로 동작합니다.

이를 통해 더욱 안정적으로 UiState 상태 관리가 가능합니다.
하나의 창구 (UiEvent, UiState)에서 상태 관리 및, 사이드 이벤트 발생 -> 단일진실 공급 원칙 준수 -> 더욱 안정적으로 UiState 상태 관리 가능

현재 아키텍쳐 특징

MVI의 예제를 보시면 ViewModel에서 Action을 받아 상태를 변경하고 있는 경우가 많습니다.
하지만 만약 Action이 20개, 30개가 된다면 어떻게 될까요?
ViewModel은 걷잡을 수 없이 뚱뚱해집니다.

보통 ViewModel이 뚱뚱해지는 경우 UseCase로 추상화를 하지만 한계가 있습니다.
UseCase는 복잡한 메서드를 추상화하는데 특화되어 있으나, 간단하지만 수많은 로직을 추상화하기는 어렵습니다.

이를 통해 저는 Handler 라는, 사용자 액션 처리를 담당하는 별도의 Delegate 객체를 두어, 분산시키는 방식을 적용하였습니다.
sideEffect를 발송, uiState를 업데이트 하기 때문에 Handler는 UseCase와 다르게 Presentation Layer입니다.

ViewModel은 초기 UiState 업데이트,
모든 Handler에서 반복되는 사이드 이펙트 발송 (observeErrorEvent)
의 역할만 남기도록 추상화했습니다.

당부

MVI로 리팩토링하면서, 이보다 더 정확한 Ui 상태를 관리법은 없는 것 같다는 느낌을 받았습니다.
기존 MVVM에는 수많은 라이브데이터를 별도의 ViewModel 메서드로 관리하고,
Android API (ViewHolder, OnBackPressDispatcher 등)에서 상태 업데이트 로직이 산발되어 있어
요구사항이 복잡해질수록 점점 스파게티 코드가 되어간다는 느낌을 받았습니다.
MVI는 이런 문제를 타파할 수 있는 확실한 해결책이라고 생각합니다.

MVI는 러닝 커브가 높습니다. 현재 코드가 이해가 굉장히 어려울 것으로 예상됩니다.
리뷰하실 때 MVI 관련하여 모르는 지식, 충돌하는 지식이 있으면 언제든지 코멘트 달아주세요.
서로 함께 성장합시다 :)

📸 이미지 첨부 (Optional)

Summary by CodeRabbit

• New Features

  • Redesigned place map interface with improved state management and real-time UI responsiveness.
  • Enhanced image preloading for faster content display.
  • Added lifecycle-aware event handling for better resource management.

• Bug Fixes

  • Improved error handling and visibility in map filtering scenarios.

• Refactor

  • Modernized architecture with consolidated Compose-based UI components.
  • Reorganized package structure for better component organization.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

PlaceMapFragment refactored from Fragment-based composition with multiple child fragments to a unified Compose-based UI. PlaceMapViewModel migrated to reactive state management using StateFlow, action/event channels, and a DI-powered handler graph. Old UI state types (PlaceListUiState, SelectedPlaceUiState) replaced with LoadState and ListLoadState. New action/handler architecture introduced for filtering, map events, and place selection. Listener interfaces reorganized to subpackage.

Changes

Cohort / File(s)	Summary
DI & Infrastructure di/placeMapHandler/{CachedPlaceByTimeTag, CachedPlaces, PlaceMapHandlerGraph, PlaceMapViewModelScope}.kt	New DI qualifiers, graph interface, and scope marker to organize handler graph creation and scoped bindings for map-handler dependencies.
UI State & Reactive Types presentation/placeMap/intent/state/{LoadState, ListLoadState, PlaceMapUiState, MapDelegate, MapManagerDelegate, StateExt}.kt	New sealed state interfaces and delegate holders replacing legacy LiveData-based state. Provides centralized UI state model and suspending await utilities for reactive flow collection.
Action/Event/Handler Architecture presentation/placeMap/intent/{action/{PlaceMapAction, FilterAction, MapEventAction, SelectAction}.kt, event/{PlaceMapEvent, MapControlEvent}.kt, handler/{ActionHandler, EventHandler, FilterActionHandler, MapEventActionHandler, MapControlEventHandler, SelectActionHandler, PlaceMapEventHandler}.kt}	New intent-driven architecture with typed actions, events, and handler implementations for filter, map, and selection flows. Replaces direct ViewModel method calls with action dispatch.
Listener Package Reorganization presentation/placeMap/listener/{MapClickListener, MapClickListenerImpl, OnCameraChangeListener}.kt, mapManager/internal/{MapCameraManagerImpl, MapMarkerManagerImpl}.kt, mapManager/{MapCameraManager, MapManager}.kt	Moved listener interfaces to new listener subpackage; updated imports across mapManager classes.
Component Styling & Constants presentation/placeMap/component/Styles.kt	Added 7 new design-system spacing extension properties (previewVerticalPadding, timeTagHorizontalPadding, previewImageSize, etc.) for UI constants.
Compose Components (Package Moves) presentation/placeMap/component/{BackToPositionButton, CurrentLocationButton, OffsetDependentLayout, PlaceListBottomSheet, PlaceListBottomSheetState, PlaceCategoryLabel, PlaceCategoryScreen, PreviewAnimatableBox}.kt	Moved from placeMap.placeList.* and placeMap.placeCategory.* subdirectories to unified placeMap.component package.
Compose Components (State Type Migrations) presentation/placeMap/component/{PlaceListScreen, PlaceDetailPreviewScreen, PlaceDetailPreviewSecondaryScreen, TimeTagMenu, NaverMapContent}.kt	Replaced PlaceListUiState with ListLoadState, SelectedPlaceUiState with LoadState; updated callbacks and state handling; NaverMapContent now accepts MapDelegate parameter and suspending map initialization.
Core Refactored Components presentation/placeMap/{PlaceMapFragment, PlaceMapViewModel}.kt	PlaceMapFragment replaced fragment transactions with Compose-based PlaceMapScreen; PlaceMapViewModel migrated to action-based architecture with StateFlow uiState, event channels, and handler graph dependency.
PlaceMapScreen & Root UI presentation/placeMap/component/PlaceMapScreen.kt	Completely rewired from prop-callback model to state-driven action model; accepts uiState: PlaceMapUiState and onAction callback; orchestrates map, time-tag menu, place list, and detail screens via state.
CategoryView Package Move presentation/placeDetail/CategoryView.kt	Moved from placeMap.placeList package to placeDetail package.
Removed Fragments presentation/placeMap/placeList/{PlaceListFragment, PlaceListViewModel, OnPlaceClickListener}.kt, presentation/placeMap/placeDetailPreview/{PlaceDetailPreviewFragment, PlaceDetailPreviewSecondaryFragment}.kt, presentation/placeMap/placeCategory/PlaceCategoryFragment.kt	Entire Fragment-based UI layer removed; functionality consolidated into Compose components orchestrated by PlaceMapFragment and PlaceMapScreen.
Removed Layout Resources res/layout/{fragment_place_category, fragment_place_detail_preview, fragment_place_detail_preview_secondary, fragment_place_list, item_place_list, item_place_list_skeleton}.xml, res/layout/activity_place_detail.xml (CategoryView reference updated)	XML layouts for removed Fragments deleted; activity_place_detail.xml updated to reference CategoryView in new package.
Removed State Models presentation/placeMap/model/{PlaceListUiState, SelectedPlaceUiState}.kt	Old sealed-interface state types removed; replaced by LoadState and ListLoadState.
Test Infrastructure & Fixtures test/java/com/daedan/festabook/placeMap/PlaceLIstTestFixture.kt	Package moved from placeList to placeMap; added FAKE_PLACES_CATEGORY_FIXTURE and FAKE_INITIAL_MAP_SETTING.
Test Utilities test/java/com/daedan/festabook/FlowExtensions.kt	New TestScope.observeEvent extension for collecting first emitted Flow item in tests.
Test Fixture Imports test/java/com/daedan/festabook/{placeDetail/PlaceDetailTestFixture, placeDetail/PlaceDetailViewModelTest}.kt	Updated import from placeList.FAKE_PLACES to placeMap.FAKE_PLACES.
Old Test Suites (Removed) test/java/com/daedan/festabook/placeList/{PlaceListViewModelTest, PlaceMapViewModelTest}.kt	Removed obsolete tests for removed PlaceListViewModel and old PlaceMapViewModel structure.
New Test Suites test/java/com/daedan/festabook/placeMap/{PlaceMapViewModelTest.kt, handler/{FilterActionHandlerTest, MapEventActionHandlerTest, SelectActionHandlerTest}.kt}	Comprehensive tests for new reactive ViewModel architecture and handler implementations.
Other Updates FestaBookApp.kt (global exception handler commented out), di/mapManager/MapManagerBindings.kt (import path updated), presentation/common/ObserveEvent.kt (new composable utility), presentation/placeMap/mapManager/internal/MapFilterManagerImpl.kt (empty time tag handling logic)	Minor updates to exception handling, imports, and utility functions.

Sequence Diagram(s)

sequenceDiagram
    participant User as User/UI
    participant Screen as PlaceMapScreen
    participant ViewModel as PlaceMapViewModel
    participant Handlers as Handler Graph<br/>(Filter/Map/Select)
    participant State as StateFlow<br/>PlaceMapUiState
    participant Channels as Event Channels<br/>(MapControl/PlaceMap)

    User->>Screen: Triggers Action<br/>(e.g., OnPlaceClick)
    Screen->>ViewModel: onPlaceMapAction(action)
    ViewModel->>Handlers: Routes to appropriate handler<br/>via handlerGraph
    
    rect rgb(240, 248, 255)
    Note over Handlers: Handler processes action
    Handlers->>Handlers: Updates internal state<br/>& computes changes
    Handlers->>State: Emits state update<br/>via onUpdateState callback
    Handlers->>Channels: Emits events<br/>(MapControlEvent, PlaceMapEvent)
    end
    
    State-->>Screen: StateFlow observes changes
    Channels-->>Screen: Event channels emit to UI
    Screen->>Screen: Re-renders based on<br/>new state & events
    Screen->>User: Updated UI

    User->>Screen: Responds to UI Event<br/>(e.g., StartPlaceDetail)
    Screen->>ViewModel: Processes event<br/>(e.g., navigate, show snackbar)

Loading

sequenceDiagram
    participant VM as PlaceMapViewModel<br/>onCreate
    participant Factory as PlaceMapHandlerGraph<br/>.Factory
    participant Graph as PlaceMapHandlerGraph
    participant Filters as FilterActionHandler
    participant MapEvents as MapEventActionHandler
    participant Selects as SelectActionHandler
    participant Cache as Cached StateFlows<br/>(Places/TimeTag)
    participant UIState as StateFlow<br/>PlaceMapUiState

    VM->>VM: Collect initial data<br/>(loadAllPlaces, observeErrorEvent)
    activate VM
    
    VM->>VM: Create cached flows<br/>(cachedPlaces, cachedPlaceByTimeTag)
    VM->>Factory: Create handler graph<br/>with dependencies
    activate Factory
    
    Factory->>Graph: Instantiate with channels,<br/>cached flows, callbacks
    activate Graph
    
    Graph->>Filters: Wire FilterActionHandler<br/>with update callbacks
    Graph->>MapEvents: Wire MapEventActionHandler<br/>with state access
    Graph->>Selects: Wire SelectActionHandler<br/>with repository & scope
    
    Graph-->>Factory: Return PlaceMapHandlerGraph
    deactivate Graph
    Factory-->>VM: Return Graph instance
    deactivate Factory
    
    VM->>VM: Store handlerGraph<br/>for action dispatch
    VM->>UIState: Initialize with<br/>Loading states
    deactivate VM
    
    Note over VM,Selects: Ready for user actions

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• PlaceMap의 하위 Fragment 제거 및 Composable 통합 #18: The changes implement the comprehensive PlaceMap Fragment-to-Compose migration and reactive ViewModel refactoring described in this issue, replacing Fragment-based composition with state-driven Compose rendering and action-based handler architecture.

Possibly related PRs

• 전역 Ktlint 적용 및 기타 작업 수행 #15: Both PRs refactor PlaceMap Compose components, ViewModel/DI structure, and theme integration; share overlapping code locations (PlaceMapFragment, PlaceMapViewModel, component hierarchy, Compose locals).

Suggested reviewers

• etama123
• parkjiminnnn

---

🐰 Fragments fade to Compose so bright,
Actions flow through handlers' might,
State now unified, no scattered views,
Reactive streams sing the refactored news!
One screen rules them all tonight. 🗺️✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 1.96% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: removing child Fragments from PlaceMap and applying MVI architecture. It directly reflects the primary refactoring objective.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[oungsi2000]

File Change 너무 죄송합니다 ㅜㅜ 최대한 노력했지만 더 작은 단위로 쪼개기가 너무 애매했어요.

[oungsi2000]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 18

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PreviewAnimatableBox.kt (1)

45-49: Exit animation comment doesn't match implementation.

The comment on line 46 describes an exit animation ("나갈 때 애니메이션"), but the code uses snapTo which resets values instantly without animating. This creates asymmetric behavior—smooth entry but instant disappearance.

If a smooth exit animation is desired, replace snapTo with animateTo. If instant dismissal is intentional, update or remove the misleading comment.

🔎 Proposed fix for smooth exit animation

         } else {
-            // 나갈 때 애니메이션 (위에서 아래로 + 페이드아웃)
-            launch { offsetY.snapTo(120f) }
-            launch { alpha.snapTo(0.3f) }
+            // 나갈 때 애니메이션 (위에서 아래로 + 페이드아웃)
+            launch { offsetY.animateTo(120f, animationSpec = tween(300)) }
+            launch { alpha.animateTo(0.3f, animationSpec = tween(300)) }
         }

Alternatively, if instant dismissal is preferred, update the comment:

         } else {
-            // 나갈 때 애니메이션 (위에서 아래로 + 페이드아웃)
+            // 즉시 숨김 상태로 리셋
             launch { offsetY.snapTo(120f) }
             launch { alpha.snapTo(0.3f) }
         }

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheet.kt (1)

92-97: Pre-existing critical issue: Coroutine launched on every layout pass causes resource leak.

The scope.launch block inside the layout modifier is executed on every layout pass (triggered by size changes, recomposition, etc.), potentially launching multiple concurrent collectors for the same state. This accumulates coroutine jobs that are never cancelled, leading to a memory leak.

🔎 Proposed fix: Move collection to LaunchedEffect

Remove the coroutine launch from inside the layout block (lines 92-97) and set up the collection in a separate LaunchedEffect outside the layout:

     val nestedScrollConnection = placeListBottomSheetNestedScrollConnection(bottomSheetState)
+
+    LaunchedEffect(bottomSheetState.state) {
+        snapshotFlow { bottomSheetState.state.requireOffset() }
+            .collect { currentOffset ->
+                onScroll(currentOffset)
+            }
+    }

     Column(
         modifier =
             modifier
                 .fillMaxSize()
                 .layout { measurable, constraints ->
                     val placeable = measurable.measure(constraints)

                     // 실제 레이아웃 측정 시에만 앵커 설정
                     if (!isLookingAhead) {
                         val screenHeightPx = constraints.maxHeight.toFloat()
                         // 3가지 앵커 높이 정의 (DP)
                         val halfExpandedOffsetPx =
                             screenHeightPx - screenHeightPx * halfExpandedRatio
                         val collapsedOffsetPx = with(density) { screenHeightPx - peekHeight.toPx() }
                         val expandedOffsetPx = 0f // 화면 최상단

                         bottomSheetState.state.updateAnchors(
                             newAnchors =
                                 DraggableAnchors {
                                     PlaceListBottomSheetValue.EXPANDED at expandedOffsetPx
                                     PlaceListBottomSheetValue.HALF_EXPANDED at halfExpandedOffsetPx
                                     PlaceListBottomSheetValue.COLLAPSED at collapsedOffsetPx
                                 },
                             newTarget = bottomSheetState.currentValue,
                         )
-                        // 스크롤 되었을 때 호출하는 콜백
-                        scope.launch {
-                            snapshotFlow { bottomSheetState.state.requireOffset() }
-                                .collect { currentOffset ->
-                                    onScroll(currentOffset)
-                                }
-                        }
                     }

This ensures the collection lifecycle is properly managed and only one collector is active per composition.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListScreen.kt (1)

151-157: LaunchedEffect(Unit) may re-trigger onPlaceLoad on recomposition.

Using Unit as the key means this effect runs once when the composable enters composition in the PlaceLoaded state. However, if the parent recomposes and PlaceListScreen is re-entered while still in PlaceLoaded, the effect will run again. Consider using a more stable key or tracking whether the load has been triggered.

🔎 Proposed fix using a derived key

 is ListLoadState.PlaceLoaded -> {
-    LaunchedEffect(Unit) {
+    LaunchedEffect(placesUiState) {
         scope.launch {
             currentOnPlaceLoad()
         }
     }
 }

This ensures the effect only re-runs when placesUiState actually changes to PlaceLoaded again.

🧹 Nitpick comments (21)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/BackToPositionButton.kt (1)

15-42: Component is well-structured.

The BackToPositionButton implementation is clean and follows Compose best practices. The hardcoded 1.dp border width at line 34 is acceptable as border widths are typically design constants. However, if you're aiming for full consistency with the spacing theme extraction mentioned in the PR objectives, you could consider adding a borderWidth constant to your theme.

app/src/main/java/com/daedan/festabook/FestaBookApp.kt (1)

72-76: Remove commented-out code.

The commented-out conditional Timber setup logic should either be removed entirely or documented if there's a specific reason to keep it for future reference.

🔎 Proposed fix

     private fun setupTimber() {
         plantDebugTimberTree()
         plantInfoTimberTree()
 
-//        if (BuildConfig.DEBUG) {
-//            plantDebugTimberTree()
-//        } else {
-//            plantInfoTimberTree()
-//        }
         Timber.plant(FirebaseCrashlyticsTree())
     }

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/EventHandler.kt (1)

3-5: Consider adding KDoc for the public interface.

As a public contract used by multiple handler implementations, KDoc would clarify the intent and usage pattern (e.g., that invoke is called to asynchronously handle an event).

📝 Suggested documentation

+/**
+ * Generic contract for asynchronous event handling.
+ * Implementations should process the given event and perform side effects as needed.
+ */
 interface EventHandler<EVENT> {
+    /**
+     * Handles the given event asynchronously.
+     * @param event the event to process
+     */
     suspend operator fun invoke(event: EVENT)
 }

app/src/main/java/com/daedan/festabook/presentation/common/ObserveEvent.kt (2)

12-25: Simplify LaunchedEffect keys and reconsider withContext.

Two observations:

1. LaunchedEffect key on lifecycleOwner.lifecycle: Including lifecycle as a key will restart the effect if the lifecycle owner changes, which is rare. Typically only flow is used as the key.

2. Redundant withContext(Dispatchers.Main.immediate): LaunchedEffect already runs on the main dispatcher in Compose, and flow.collect(onEvent) invokes onEvent in the collector's context (already main). The withContext is defensive but likely unnecessary unless the flow uses flowOn() internally.

🔎 Proposed simplification

 @Composable
 fun <T> ObserveAsEvents(
     flow: Flow<T>,
     onEvent: suspend (T) -> Unit,
 ) {
     val lifecycleOwner = LocalLifecycleOwner.current
-    LaunchedEffect(flow, lifecycleOwner.lifecycle) {
-        lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) {
-            withContext(Dispatchers.Main.immediate) {
-                flow.collect(onEvent)
-            }
+    LaunchedEffect(flow) {
+        lifecycleOwner.lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) {
+            flow.collect(onEvent)
         }
     }
 }

---

12-16: Consider adding KDoc for the public Composable.

This utility establishes a lifecycle-aware event observation pattern. Documenting the behavior (especially that events are only collected while STARTED) would help consumers understand when to use it versus other state collection mechanisms.

app/src/test/java/com/daedan/festabook/placeMap/PlaceLIstTestFixture.kt (1)

50-86: Consider refactoring duplicate fixture structure.

FAKE_PLACES_CATEGORY_FIXTURE largely duplicates FAKE_PLACES (lines 12-48), differing only in the category of the second place. If category variation is the primary test concern, consider a factory function or parameterized fixture to reduce duplication.

💡 Example factory approach

fun createFakePlaces(secondPlaceCategory: PlaceCategory = PlaceCategory.FOOD_TRUCK): List<Place> =
    listOf(
        Place(
            id = 1,
            category = PlaceCategory.FOOD_TRUCK,
            // ... other fields
        ),
        Place(
            id = 2,
            category = secondPlaceCategory,
            // ... other fields
        ),
    )

val FAKE_PLACES = createFakePlaces()
val FAKE_PLACES_CATEGORY_FIXTURE = createFakePlaces(PlaceCategory.BAR)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/Styles.kt (1)

1-25: LGTM!

Extracting hardcoded dimensions into themed extension properties is a solid approach. The naming is clear, and grouping these in Styles.kt keeps design tokens centralized.

Optional: Add a file-level KDoc summarizing the spacing constants for PlaceMap components, especially since placeListBottomSheetHalfRatio is a ratio (Float) rather than a dimension (Dp).

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/ActionHandler.kt (1)

7-7: Consider renaming lambda parameter for clarity.

The parameter name before in the function type ((before: STATE) -> STATE) suggests temporal ordering, but it represents the current state being transformed. Consider current or simply state for better clarity in IDE hints and documentation.

🔎 Suggested naming improvement

-    val onUpdateState: ((before: STATE) -> STATE) -> Unit
+    val onUpdateState: ((current: STATE) -> STATE) -> Unit

or simply:

-    val onUpdateState: ((before: STATE) -> STATE) -> Unit
+    val onUpdateState: ((STATE) -> STATE) -> Unit

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/MapDelegate.kt (1)

23-29: Consider reordering Flow operators for clarity.

The current operator order applies distinctUntilChanged() before filterNotNull(), which means it compares nullable values. While this works correctly for the single-initialization use case, the more idiomatic pattern is:

snapshotFlow { value }
    .filterNotNull()
    .distinctUntilChanged()
    .first()

This ensures distinctUntilChanged() only compares non-null NaverMap instances, making the intent clearer.

🔎 Suggested operator reordering

 suspend fun await(timeout: Duration = 3.seconds): NaverMap =
     withTimeout(timeout) {
         snapshotFlow { value }
-            .distinctUntilChanged()
             .filterNotNull()
+            .distinctUntilChanged()
             .first()
     }

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/ListLoadState.kt (1)

12-14: Consider removing PlaceLoaded in favor of Success<List<PlaceUiModel>>.

PlaceLoaded appears to duplicate the purpose of Success<List<PlaceUiModel>>. Having both variants can confuse consumers about which to use and complicates exhaustive when expressions.

If PlaceLoaded serves a semantic purpose (e.g., triggering specific side effects), document that distinction; otherwise, remove it to keep the API surface minimal.

app/src/test/java/com/daedan/festabook/placeMap/handler/FilterActionHandlerTest.kt (1)

107-147: Duplicate test cases.

These two tests (선택된 카테고리가 부스, 주점, 푸드트럭에 해당되지 않을 때 전체 목록을 불러온다 and 기타 카테고리만 선택되었다면 전체 목록을 불러온다) test the same scenario with identical targetCategories (SMOKING_AREA and TOILET). Consider removing one or differentiating them with distinct category combinations.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceMapScreen.kt (2)

69-86: Consider conditional rendering instead of alpha(0f) for performance.

Using alpha(0f) keeps PlaceListScreen in the composition tree even when hidden, potentially causing unnecessary layout and state management overhead. If the component is not visible, conditional rendering would be more efficient.

🔎 Proposed approach

-                PlaceListScreen(
-                    modifier =
-                        Modifier.alpha(
-                            if (uiState.selectedPlace is LoadState.Empty) {
-                                1f
-                            } else {
-                                0f
-                            },
-                        ),
+                if (uiState.selectedPlace is LoadState.Empty) {
+                    PlaceListScreen(
+                        modifier = Modifier,
                     ...
-                )
+                    )
+                }

However, if you need to preserve scroll state or avoid recomposition costs when toggling, the current approach is acceptable.

---

88-117: Redundant visible = true parameters.

Both PlaceDetailPreviewScreen and PlaceDetailPreviewSecondaryScreen are conditionally rendered based on uiState.isPlacePreviewVisible and uiState.isPlaceSecondaryPreviewVisible, making the visible = true parameter redundant. Consider removing it or relying solely on one mechanism.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/NaverMapContent.kt (1)

160-168: Missing cancellation handling in suspendCancellableCoroutine.

If the coroutine is cancelled before getMapAsync completes, the callback will still execute and attempt to resume a cancelled continuation. Consider adding invokeOnCancellation or using resume with a check.

🔎 Proposed improvement

 private suspend fun MapView.getMapAndRunCallback(onMapReady: (NaverMap) -> Unit = {}): NaverMap =
     suspendCancellableCoroutine { continuation ->
         getMapAsync { map ->
-            onMapReady(map)
-            continuation.resumeWith(
-                Result.success(map),
-            )
+            if (continuation.isActive) {
+                onMapReady(map)
+                continuation.resume(map)
+            }
         }
     }

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/FilterActionHandler.kt (2)

81-84: Consider making unselectPlace a private function.

The unselectPlace function is private, which is correct. However, trySend on line 83 silently drops the event if the channel buffer is full. For critical UI state changes, consider logging or handling the case when trySend fails.

🔎 Proposed enhancement

 private fun unselectPlace() {
     onUpdateState.invoke { it.copy(selectedPlace = LoadState.Empty) }
-    _mapControlUiEvent.trySend(MapControlEvent.UnselectMarker)
+    _mapControlUiEvent.trySend(MapControlEvent.UnselectMarker).also { result ->
+        if (result.isFailure) {
+            // Consider logging: marker unselect event dropped
+        }
+    }
 }

---

71-73: Fix typo in constant name: EMTPY_TIME_TAG_ID should be EMPTY_TIME_TAG_ID.

The constant TimeTag.EMTPY_TIME_TAG_ID is misspelled (should be EMPTY). This typo is defined in the domain model and consistently used across multiple files (FilterActionHandler.kt, MapControlEventHandler.kt, MapFilterManagerImpl.kt, and others). Fixing it requires updating the constant definition in TimeTag.kt and all 8 usages throughout the presentation layer.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/TimeTagMenu.kt (1)

54-73: Good state-driven pattern, but consider handling Loading/Error states explicitly.

The TimeTagMenu now correctly uses LoadState wrappers. However, the else -> Unit branch silently ignores Loading and Error states. Consider whether users should see a loading indicator or error message when time tags fail to load.

🔎 Optional: Add loading/error UI feedback

     when (timeTagsState) {
         is LoadState.Success -> {
             if (selectedTimeTagState !is LoadState.Success) return
             TimeTagContent(
                 title = selectedTimeTagState.value.name,
                 timeTags = timeTagsState.value,
                 modifier = modifier,
                 onTimeTagClick = onTimeTagClick,
             )
         }
-
-        else -> Unit
+        is LoadState.Loading -> {
+            // Optional: Show shimmer or placeholder
+        }
+        is LoadState.Error -> {
+            // Optional: Show error indicator
+        }
+        is LoadState.Empty -> Unit
     }

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/SelectActionHandler.kt (2)

113-117: Parameter naming: item should be error or throwable.

The onFailure callback receives a Throwable, but the parameter is named item, which is misleading.

🔎 Proposed fix

-            }.onFailure { item ->
+            }.onFailure { error ->
                 onUpdateState.invoke {
-                    it.copy(selectedPlace = LoadState.Error(item))
+                    it.copy(selectedPlace = LoadState.Error(error))
                 }
             }

---

27-36: Handler-to-handler dependency creates tight coupling.

SelectActionHandler depends directly on FilterActionHandler (line 30) to call updatePlacesByTimeTag. While this works, it creates tight coupling between handlers. Consider extracting shared logic into a common service or using events for cross-handler communication.

app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapViewModel.kt (2)

52-62: Channel overflow strategy may silently drop events.

Both _placeMapUiEvent and _mapControlUiEvent use BufferOverflow.DROP_OLDEST, which silently discards events when the buffer is full. For UI events like ShowErrorSnackBar or map control commands, consider using DROP_LATEST (drop new events) or increasing buffer capacity to avoid losing important events.

🔎 Alternative with explicit capacity

     private val _placeMapUiEvent =
         Channel<PlaceMapEvent>(
+            capacity = Channel.BUFFERED,
             onBufferOverflow = BufferOverflow.DROP_OLDEST,
         )

---

186-200: Redundant nested launch.

The inner launch at line 189 is unnecessary since the outer launch already provides the coroutine scope. This adds minor overhead and reduces readability.

🔎 Proposed simplification

     @OptIn(FlowPreview::class)
     private fun observeErrorEvent() {
         viewModelScope.launch {
-            launch {
-                uiState
-                    .map { it.hasAnyError }
-                    .distinctUntilChanged()
-                    .filterIsInstance<LoadState.Error>()
-                    .debounce(1000)
-                    .collect {
-                        _placeMapUiEvent.send(PlaceMapEvent.ShowErrorSnackBar(it))
-                    }
-            }
+            uiState
+                .map { it.hasAnyError }
+                .distinctUntilChanged()
+                .filterIsInstance<LoadState.Error>()
+                .debounce(1000)
+                .collect {
+                    _placeMapUiEvent.send(PlaceMapEvent.ShowErrorSnackBar(it))
+                }
         }
     }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ea2cc3 and f71dc7b.

📒 Files selected for processing (77)

• app/src/main/java/com/daedan/festabook/FestaBookApp.kt
• app/src/main/java/com/daedan/festabook/di/mapManager/MapManagerBindings.kt
• app/src/main/java/com/daedan/festabook/di/placeMapHandler/CachedPlaceByTimeTag.kt
• app/src/main/java/com/daedan/festabook/di/placeMapHandler/CachedPlaces.kt
• app/src/main/java/com/daedan/festabook/di/placeMapHandler/PlaceMapHandlerGraph.kt
• app/src/main/java/com/daedan/festabook/di/placeMapHandler/PlaceMapViewModelScope.kt
• app/src/main/java/com/daedan/festabook/presentation/common/ObserveEvent.kt
• app/src/main/java/com/daedan/festabook/presentation/placeDetail/CategoryView.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapViewModel.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/BackToPositionButton.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/CurrentLocationButton.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/NaverMapContent.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/OffsetDependentLayout.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceCategoryLabel.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceCategoryScreen.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewScreen.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewSecondaryScreen.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheet.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheetState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListScreen.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceMapScreen.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PreviewAnimatableBox.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/Styles.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/component/TimeTagMenu.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/FilterAction.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/MapEventAction.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/PlaceMapAction.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/SelectAction.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/event/MapControlEvent.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/event/PlaceMapEvent.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/ActionHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/EventHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/FilterActionHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/MapControlEventHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/MapEventActionHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/PlaceMapEventHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/SelectActionHandler.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/ListLoadState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/LoadState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/MapDelegate.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/MapManagerDelegate.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/PlaceMapUiState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/StateExt.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/MapClickListener.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/MapClickListenerImpl.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/OnCameraChangeListener.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/MapCameraManager.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/MapManager.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/internal/MapCameraManagerImpl.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/internal/MapFilterManagerImpl.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/internal/MapMarkerManagerImpl.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/model/PlaceListUiState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/model/SelectedPlaceUiState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeCategory/PlaceCategoryFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeDetailPreview/PlaceDetailPreviewFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeDetailPreview/PlaceDetailPreviewSecondaryFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/OnPlaceClickListener.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/PlaceListFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/PlaceListViewModel.kt
• app/src/main/res/layout/activity_place_detail.xml
• app/src/main/res/layout/fragment_place_category.xml
• app/src/main/res/layout/fragment_place_detail_preview.xml
• app/src/main/res/layout/fragment_place_detail_preview_secondary.xml
• app/src/main/res/layout/fragment_place_list.xml
• app/src/main/res/layout/item_place_list.xml
• app/src/main/res/layout/item_place_list_skeleton.xml
• app/src/test/java/com/daedan/festabook/FlowExtensions.kt
• app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailTestFixture.kt
• app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailViewModelTest.kt
• app/src/test/java/com/daedan/festabook/placeList/PlaceListViewModelTest.kt
• app/src/test/java/com/daedan/festabook/placeList/PlaceMapViewModelTest.kt
• app/src/test/java/com/daedan/festabook/placeMap/PlaceLIstTestFixture.kt
• app/src/test/java/com/daedan/festabook/placeMap/PlaceMapViewModelTest.kt
• app/src/test/java/com/daedan/festabook/placeMap/handler/FilterActionHandlerTest.kt
• app/src/test/java/com/daedan/festabook/placeMap/handler/MapEventActionHandlerTest.kt
• app/src/test/java/com/daedan/festabook/placeMap/handler/SelectActionHandlerTest.kt

💤 Files with no reviewable changes (16)

• app/src/main/res/layout/fragment_place_detail_preview.xml
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/OnPlaceClickListener.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/model/PlaceListUiState.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/PlaceListViewModel.kt
• app/src/main/res/layout/fragment_place_detail_preview_secondary.xml
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeCategory/PlaceCategoryFragment.kt
• app/src/main/res/layout/item_place_list.xml
• app/src/test/java/com/daedan/festabook/placeList/PlaceListViewModelTest.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeList/PlaceListFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/model/SelectedPlaceUiState.kt
• app/src/main/res/layout/item_place_list_skeleton.xml
• app/src/test/java/com/daedan/festabook/placeList/PlaceMapViewModelTest.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeDetailPreview/PlaceDetailPreviewFragment.kt
• app/src/main/java/com/daedan/festabook/presentation/placeMap/placeDetailPreview/PlaceDetailPreviewSecondaryFragment.kt
• app/src/main/res/layout/fragment_place_list.xml
• app/src/main/res/layout/fragment_place_category.xml

🧰 Additional context used

🧬 Code graph analysis (6)

app/src/test/java/com/daedan/festabook/placeMap/handler/FilterActionHandlerTest.kt (2)

app/src/test/java/com/daedan/festabook/FlowExtensions.kt (1)

• observeEvent (11-19)

app/src/main/java/com/daedan/festabook/di/placeMapHandler/PlaceMapHandlerGraph.kt (1)

• filterActionHandler (18-38)

app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapFragment.kt (3)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheetState.kt (1)

• rememberPlaceListBottomSheetState (63-75)

app/src/main/java/com/daedan/festabook/presentation/common/ObserveEvent.kt (1)

• ObserveAsEvents (12-25)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceMapScreen.kt (1)

• PlaceMapScreen (23-121)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceMapScreen.kt (7)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/NaverMapContent.kt (1)

• NaverMapContent (26-46)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/TimeTagMenu.kt (1)

• TimeTagMenu (52-73)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceCategoryScreen.kt (1)

• PlaceCategoryScreen (30-83)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/NaverMapLogo.kt (1)

• NaverMapLogo (10-18)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListScreen.kt (1)

• PlaceListScreen (51-161)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewScreen.kt (1)

• PlaceDetailPreviewScreen (33-62)

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewSecondaryScreen.kt (1)

• PlaceDetailPreviewSecondaryScreen (31-92)

app/src/test/java/com/daedan/festabook/placeMap/PlaceMapViewModelTest.kt (1)

app/src/test/java/com/daedan/festabook/FlowExtensions.kt (1)

• observeEvent (11-19)

app/src/test/java/com/daedan/festabook/placeMap/handler/MapEventActionHandlerTest.kt (1)

app/src/test/java/com/daedan/festabook/FlowExtensions.kt (1)

• observeEvent (11-19)

app/src/test/java/com/daedan/festabook/placeMap/handler/SelectActionHandlerTest.kt (1)

app/src/test/java/com/daedan/festabook/FlowExtensions.kt (1)

• observeEvent (11-19)

🔇 Additional comments (66)

app/src/main/java/com/daedan/festabook/di/mapManager/MapManagerBindings.kt (1)

4-5: LGTM! Clean package reorganization.

The import paths have been correctly updated to reflect the new listener subpackage structure. The DI binding logic at line 28 remains consistent with these changes.

app/src/main/java/com/daedan/festabook/presentation/placeDetail/CategoryView.kt (1)

1-1: LGTM! Package refactoring aligns with usage context.

The package move from placeMap.placeList to placeDetail correctly reflects where this component is actually used (in the place detail screen). The refactoring maintains the import of PlaceCategoryUiModel from the placeMap.model package, which appears intentional for keeping shared models in their original location.

app/src/main/res/layout/activity_place_detail.xml (1)

79-79: LGTM! XML reference correctly updated.

The CategoryView reference has been properly updated to reflect the new package location (com.daedan.festabook.presentation.placeDetail.CategoryView), maintaining consistency with the refactored Kotlin class.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/BackToPositionButton.kt (1)

1-1: LGTM! Package reorganization aligns with component consolidation.

The package move from placeList.component to placeMap.component is consistent with the broader refactoring to consolidate PlaceMap-related components under a unified namespace.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PreviewAnimatableBox.kt (1)

1-1: Package consolidation looks good.

The move to the flatter placeMap.component package aligns with the PR's goal of reorganizing and consolidating UI components.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/CurrentLocationButton.kt (1)

1-1: Package reorganization is complete.

The package refactor from placeMap.placeList.component to placeMap.component aligns with the PR's goal of consolidating Compose components into a unified namespace, and all references have been properly updated across the codebase.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/OffsetDependentLayout.kt (1)

1-1: Package consolidation is complete—no broken imports.

All usages of OffsetDependentLayout have been updated. The component is only used in PlaceListScreen.kt, which is in the same consolidated package (com.daedan.festabook.presentation.placeMap.component), so no explicit imports are needed. No lingering references to the old package path exist.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheet.kt (1)

1-1: LGTM! Package consolidation aligns with refactoring goals.

The package rename from placeMap.placeList.component to placeMap.component appropriately reflects the removal of nested fragment structure and consolidates components into a unified package.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListBottomSheetState.kt (1)

1-1: LGTM! Package consolidation aligns with the component-based architecture.

The package move from placeMap.placeList.component to placeMap.component consolidates UI components under a unified namespace, which simplifies the structure and aligns with the broader Compose refactor.

app/src/main/java/com/daedan/festabook/di/placeMapHandler/CachedPlaces.kt (1)

1-6: LGTM! Standard DI qualifier for disambiguating cached place dependencies.

The @CachedPlaces qualifier follows proper Metro DI conventions and will help distinguish between different StateFlow instances in the handler graph.

app/src/main/java/com/daedan/festabook/di/placeMapHandler/PlaceMapViewModelScope.kt (1)

1-3: LGTM! Proper scope marker implementation for DI.

The abstract class with private constructor correctly implements the scope marker pattern, ensuring it can only be used as a type-level scope indicator in the DI graph.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceCategoryScreen.kt (1)

1-1: LGTM! Package consolidation consistent with component reorganization.

Moving from placeMap.placeCategory.component to placeMap.component aligns with the unified component namespace established in this refactor.

app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/OnCameraChangeListener.kt (1)

1-1: LGTM! Listener interfaces properly organized into dedicated subpackage.

Moving camera listener to the placeMap.listener subpackage improves code organization by grouping related listener interfaces together.

app/src/test/java/com/daedan/festabook/placeMap/PlaceLIstTestFixture.kt (1)

163-178: LGTM!

The FAKE_INITIAL_MAP_SETTING fixture is well-structured and provides clear test data for map initialization scenarios.

app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/MapCameraManager.kt (1)

3-3: LGTM!

Clean package reorganization—moving OnCameraChangeListener to the listener subpackage improves structure.

app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailViewModelTest.kt (1)

7-7: LGTM! Test fixture import updated correctly.

The import path change aligns with the test fixture reorganization to the placeMap package. The test logic remains unchanged and continues to validate PlaceDetailViewModel behavior correctly.

app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/internal/MapMarkerManagerImpl.kt (1)

4-4: LGTM! Package reorganization applied correctly.

The MapClickListener import has been updated to reflect the new listener subpackage structure. No functional changes.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/PlaceMapAction.kt (1)

1-3: LGTM! Clean MVI action hierarchy base.

The sealed interface provides a type-safe foundation for the MVI action hierarchy, enabling the ViewModel to route FilterAction, MapEventAction, and SelectAction through a unified entry point.

app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/internal/MapCameraManagerImpl.kt (1)

4-4: LGTM! Package reorganization applied correctly.

The OnCameraChangeListener import has been updated to reflect the new listener subpackage structure. No functional changes.

app/src/main/java/com/daedan/festabook/presentation/placeMap/mapManager/MapManager.kt (1)

7-7: LGTM! Package reorganization applied correctly.

The MapClickListener import has been updated to reflect the new listener subpackage structure. This completes the consistent reorganization of listener interfaces across all map manager components.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceCategoryLabel.kt (1)

24-24: Good refactor to use theme-based spacing.

Replacing the hard-coded 4.dp with festabookSpacing.paddingBody1 improves maintainability and aligns with the design system standardization introduced in this PR.

Also applies to: 55-55

app/src/main/java/com/daedan/festabook/di/placeMapHandler/CachedPlaceByTimeTag.kt (1)

1-6: LGTM! Clean DI qualifier implementation.

The annotation is correctly defined using Metro's @Qualifier to distinguish cached place data by time tag. This complements the existing CachedPlaces qualifier and supports the PR's handler-based DI architecture.

app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/MapClickListener.kt (1)

1-1: Good organizational refactor.

Moving the listener interface to a dedicated listener subpackage improves code organization and makes the package structure more logical.

app/src/main/java/com/daedan/festabook/presentation/placeMap/listener/MapClickListenerImpl.kt (1)

1-4: Excellent MVI refactor!

The migration from direct ViewModel method calls (selectPlace, unselectPlace) to action-based dispatch (onPlaceMapAction) is a clean implementation of the MVI pattern. This centralizes user interactions through a single action channel, improving testability and making the data flow more traceable.

Also applies to: 16-18, 24-24

app/src/test/java/com/daedan/festabook/placeDetail/PlaceDetailTestFixture.kt (1)

9-9: No action required. The import path change to com.daedan.festabook.placeMap.FAKE_PLACES is correct. FAKE_PLACES is defined at the new location as a non-empty List<Place> and supports the .first() call used at line 15.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/MapManagerDelegate.kt (1)

8-14: Clean delegate implementation.

The MapManagerDelegate follows good encapsulation practices with a public readable property and controlled mutation via init(). The use of mutableStateOf ensures Compose reactivity.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/FilterAction.kt (1)

5-11: LGTM!

Clean and idiomatic sealed interface definition for filter-related actions. The use of data class for OnCategoryClick (with state) and data object for OnPlaceLoad (stateless) is appropriate for the MVI action pattern.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/MapEventAction.kt (1)

5-15: LGTM!

Well-structured sealed interface for map event actions, consistent with the action pattern used elsewhere in the PR. The distinction between stateless (data object) and stateful (data class) actions is correctly applied.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/action/SelectAction.kt (1)

7-27: LGTM!

Well-structured action hierarchy covering selection, navigation, and state-related user intents. The use of domain types (TimeTag, PlaceDetailUiModel) maintains type safety.

app/src/test/java/com/daedan/festabook/placeMap/handler/SelectActionHandlerTest.kt (1)

77-101: Good test coverage for the happy path.

Test correctly verifies repository interaction, UI state update, and event emission for OnPlaceClick. The use of observeEvent to capture async events is appropriate.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/StateExt.kt (1)

9-14: Potential indefinite suspension if R is never emitted.

first() will suspend forever if the selector never produces an instance of R. Consider adding a timeout or documenting this behavior clearly for callers.

// Example: This will hang if selectedPlace never becomes Success
val place: LoadState.Success<PlaceDetailUiModel> = uiState.await { it.selectedPlace }

If indefinite suspension is acceptable by design, add KDoc documenting the contract. Otherwise, consider firstOrNull() with a timeout:

suspend inline fun <reified R> StateFlow<PlaceMapUiState>.await(
    timeout: Duration = 30.seconds,
    crossinline selector: (PlaceMapUiState) -> Any?
): R = withTimeout(timeout) {
    this@await
        .map { selector(it) }
        .distinctUntilChanged()
        .filterIsInstance<R>()
        .first()
}

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/LoadState.kt (2)

6-18: LGTM!

Well-designed sealed interface with proper covariance (out T). Using data object for Loading and Empty ensures stable equality semantics, and LoadState<Nothing> for Error correctly supports covariant type projections.

---

20-20: SECONDARY_CATEGORIES is properly defined in PlaceCategoryUiModel.Companion.

The constant is defined at PlaceCategoryUiModel.kt:28 within the companion object and is correctly referenced in the extension property. No action needed.

app/src/test/java/com/daedan/festabook/placeMap/handler/FilterActionHandlerTest.kt (2)

79-105: LGTM!

The test correctly validates that category selection triggers a FilterMapByCategory event and updates the UI state. The assertion structure with separate state and event verification is appropriate.

---

149-228: LGTM!

The remaining tests provide good coverage for filter release, time tag filtering, and the OnPlaceLoad action. The test setup properly initializes state before exercising the handler.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/event/PlaceMapEvent.kt (1)

7-27: LGTM!

Clean sealed interface design for one-shot UI events. The use of LoadState.Success and LoadState.Error wrappers provides type safety and enforces correct state at the event boundary.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/MapEventActionHandler.kt (1)

36-59: LGTM!

The remaining action handlers are well-implemented. The logging in OnBackToInitialPositionClick follows a consistent pattern, and the visibility computation in OnMapDrag correctly combines both preview states.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceMapScreen.kt (1)

23-30: LGTM on overall screen structure!

The function signature is clean with a single onAction callback for all user interactions, following MVI principles. The state-driven rendering from PlaceMapUiState provides a clear, predictable UI flow.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/event/MapControlEvent.kt (2)

19-23: Consider if LoadState<TimeTag> is necessary for selectedTimeTag.

SetMarkerByTimeTag carries selectedTimeTag: LoadState<TimeTag>, but if this event is only emitted when a time tag is successfully selected, a plain TimeTag (or TimeTag?) might be simpler. Verify that consumers need to handle Loading/Error/Empty states for this field.

---

10-34: LGTM!

The sealed interface provides a comprehensive set of map control events. Using data object for stateless events (InitMap, BackToInitialPosition, UnselectMarker) and data class for events with payloads is idiomatic Kotlin.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/NaverMapContent.kt (1)

36-45: LGTM!

The map initialization flow is well-structured: LaunchedEffect keyed on mapView ensures single initialization, the suspending getMapAndRunCallback invokes onMapReady, then the delegate is initialized. The content slot receives the map from mapDelegate.value.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/state/PlaceMapUiState.kt (3)

27-37: LGTM!

The hasAnyError aggregator provides a clean way to detect the first error across multiple state fields. Converting ListLoadState.Error to LoadState.Error maintains consistency in error handling.

---

10-19: LGTM on state structure!

The PlaceMapUiState provides a comprehensive single source of truth for the place-map UI. Sensible defaults (LoadState.Loading, LoadState.Empty) and the separation between LoadState (single values) and ListLoadState (collections) is appropriate.

---

21-25: No action required. The isSecondary property is correctly defined as an extension property on LoadState.Success<PlaceDetailUiModel> in LoadState.kt (line 20). The code at lines 21-25 will compile and execute correctly.

Likely an incorrect or invalid review comment.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/PlaceMapEventHandler.kt (2)

13-24: Well-structured event handler with clean dependency injection.

The constructor clearly separates Android-dependent callbacks from platform-agnostic dependencies. The TODO comment on line 19 appropriately flags future Compose migration work.

---

26-56: LGTM on event handling logic.

The when expression cleanly dispatches each event type. The early returns in MenuItemReClicked (line 34) and MapViewDrag (line 52) correctly guard against unnecessary operations when conditions aren't met.

app/src/main/java/com/daedan/festabook/di/placeMapHandler/PlaceMapHandlerGraph.kt (1)

18-37: Clean DI graph structure using Metro's graph extension pattern.

The PlaceMapHandlerGraph properly separates handler creation into a factory with explicit @Provides annotations. The use of qualifier annotations (@CachedPlaces, @CachedPlaceByTimeTag) correctly disambiguates the two StateFlow<List<PlaceUiModel>> parameters.

app/src/test/java/com/daedan/festabook/placeMap/PlaceMapViewModelTest.kt (1)

239-252: Good test for event emission using observeEvent helper.

The test correctly uses the observeEvent extension function from FlowExtensions.kt to observe the placeMapUiEvent channel and verify the MenuItemReClicked event is emitted.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/TimeTagMenu.kt (1)

148-152: Good use of design system spacing constant.

The hardcoded width value has been replaced with festabookSpacing.timeTagButtonWidth, improving maintainability and consistency.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceListScreen.kt (2)

99-101: Good migration to design system constants.

The bottom sheet configuration now uses festabookSpacing.placeListBottomSheetPeekHeight and festabookSpacing.placeListBottomSheetHalfRatio instead of hardcoded values, improving consistency.

---

73-97: Conditional layout visibility based on bottom sheet state.

The OffsetDependentLayout is now hidden when the bottom sheet is EXPANDED. This is a sensible UX improvement to avoid UI clutter when the list is fully expanded.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/FilterActionHandler.kt (1)

86-97: updatePlacesByTimeTag is public but only used internally and by SelectActionHandler.

This cross-handler dependency is a design choice. Consider documenting this coupling or extracting shared logic to a common utility if more handlers need this functionality in the future.

app/src/main/java/com/daedan/festabook/presentation/placeMap/intent/handler/MapControlEventHandler.kt (4)

31-41: LGTM!

The InitMap handler correctly awaits the map, registers a location listener for analytics, and assigns the location source.

---

43-60: LGTM!

The guard if (mapManager == null) ensures one-time initialization, and mapManager?.setupBackToInitialPosition correctly re-evaluates the delegate after init().

---

66-84: LGTM!

Marker setup and time-tag filtering logic handles Success and Empty states appropriately, with safe fallthrough for other states.

---

86-124: LGTM!

Category filtering and marker selection/unselection logic is correctly implemented with null-safe mapManager access.

app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapFragment.kt (3)

93-115: Handler creation in remember may miss recomposition-safe dependencies.

MapControlEventHandler and PlaceMapEventHandler are created in remember {} without keys. While this avoids recreation, some dependencies (e.g., density, appGraph.defaultFirebaseLogger) are stable, but locationSource, placeMapViewModel, and callback lambdas are captured by reference. If any of these change across the Fragment lifecycle, the handlers will hold stale references.

Consider using rememberUpdatedState for callbacks or passing stable references via DI to ensure handler instances remain valid.

---

117-132: LGTM!

Event observation via ObserveAsEvents and the PlaceMapScreen composition are correctly wired with proper state collection and action dispatch.

---

137-140: LGTM!

Menu re-click correctly dispatches the UnSelectPlace action before triggering the re-click event.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewSecondaryScreen.kt (2)

41-51: LGTM!

BackHandler is correctly scoped to visible state, and click handling properly forwards the current selectedPlace.

---

58-90: LGTM!

The Success branch correctly renders the category icon and place title with proper theme spacing.

app/src/main/java/com/daedan/festabook/presentation/placeMap/PlaceMapViewModel.kt (2)

64-92: LGTM!

Handler graph creation via factory and action routing in onPlaceMapAction provide clean separation of concerns. The when expression ensures exhaustive handling of all PlaceMapAction subtypes.

---

146-184: LGTM!

Parallel loading of organization geography and place geographies via nested launch is appropriate. State updates correctly wrap results in LoadState.

app/src/main/java/com/daedan/festabook/presentation/placeMap/component/PlaceDetailPreviewScreen.kt (3)

42-50: LGTM!

BackHandler and click handling are correctly implemented, consistent with PlaceDetailPreviewSecondaryScreen.

---

64-168: LGTM!

PlaceDetailPreviewContent correctly renders place details with consistent theme spacing and proper null handling for optional fields.

---

170-195: LGTM!

formattedDate helper handles null times gracefully, and preview correctly uses LoadState.Success with test fixtures.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Minor typo in test name.

Line 104: "떄" should be "때" (카테고리가 기타시설일 때에도).

🤖 Prompt for AI Agents

In
app/src/test/java/com/daedan/festabook/placeMap/handler/SelectActionHandlerTest.kt
around lines 103 to 125, the test function name contains a typo: change "떄" to
the correct Korean "때" in the test name declaration so the function reads `fun
\`카테고리가 기타시설일 때에도 플레이스 상세를 선택할 수 있다\`() = ...`; update the function name only
(no behavior changes) and run tests to ensure naming compiles.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test verifies handler property access, not handler invocation.

The coVerify calls check that fakeHandlerGraph.filterActionHandler etc. were accessed (getter called), but don't verify that the handler's invoke method was actually called with the action. Consider verifying the actual handler invocation.

🔎 Proposed fix to verify handler invocation

-            coVerify(exactly = 1) { fakeHandlerGraph.filterActionHandler }
-            coVerify(exactly = 1) { fakeHandlerGraph.selectActionHandler }
-            coVerify(exactly = 1) { fakeHandlerGraph.mapEventActionHandler }
+            val mockFilterHandler = mockk<FilterActionHandler>(relaxed = true)
+            val mockSelectHandler = mockk<SelectActionHandler>(relaxed = true)
+            val mockMapEventHandler = mockk<MapEventActionHandler>(relaxed = true)
+            every { fakeHandlerGraph.filterActionHandler } returns mockFilterHandler
+            every { fakeHandlerGraph.selectActionHandler } returns mockSelectHandler
+            every { fakeHandlerGraph.mapEventActionHandler } returns mockMapEventHandler
+            // Then verify invoke was called
+            coVerify { mockSelectHandler.invoke(SelectAction.UnSelectPlace) }
+            coVerify { mockFilterHandler.invoke(FilterAction.OnPlaceLoad) }
+            coVerify { mockMapEventHandler.invoke(MapEventAction.OnMapDrag) }

Note: The mock setup would need to be moved to before ViewModel creation for this to work properly.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In app/src/test/java/com/daedan/festabook/placeMap/PlaceMapViewModelTest.kt
around lines 233 to 237, the test currently verifies only that handler
properties were accessed (getters), not that the handlers were invoked; change
the assertions to verify the handler invocation instead (e.g., coVerify that
fakeHandlerGraph.filterActionHandler.invoke(...) was called with the expected
action or any()), and do the same for selectActionHandler and
mapEventActionHandler; to make this work move or create the mock handler
instances and their stubbing before constructing the ViewModel so the ViewModel
holds the mock handlers and the coVerify checks the invoke calls rather than
property access.