refactor: #87 LocationService Client 모듈 전환 #88
Conversation
📝 WalkthroughWalkthroughRefactors location handling by removing the shared singleton service and adding a new Client.Location module (LocationClient + LocationService), wires the dependency into the Map feature, and updates Tuist templates and project manifests to expose the new client and package dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Map as MapFeature.LocationFeature
participant Client as LocationClient (Dependency)
participant Service as LocationService (MainActor)
participant OS as CLLocationManager
rect rgb(230, 248, 255)
Map->>Client: requestUserLocation() (async)
Client->>Service: await requestUserLocation()
Service->>OS: requestWhenInUseAuthorization / startUpdatingLocation
Note right of OS: User grants/denies or location updates arrive
end
alt authorization granted & location obtained
OS-->>Service: didUpdateLocations(CLLocation)
Service->>Service: convert to Coordinate, stop updates
Service-->>Client: return Coordinate?
Client-->>Map: return Coordinate?
Note right of Map: update map state / moveToUserLocation
else denied or unavailable
OS-->>Service: didChangeAuthorization / no location
Service-->>Client: return nil
Client-->>Map: return nil
Note right of Map: present alert / toggle UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Projects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift (1)
77-89: Add error handling for the throwingrequestUserLocation()call.The
locationClient.requestUserLocation()method is declared as throwing (async throws -> Coordinate?), but the code usestry awaitwithout acatchblock. Throughout the codebase, all throwing async calls inEffect.runare wrapped in explicitdo-catcherror handling. Add ado-catchblock to handle potential errors consistently with established patterns.Proposed fix
private func moveUserLocation(isCurrentButtonTap: Bool) -> Effect<Action> { return .run { send in - if let location = try await locationClient.requestUserLocation() { - await send(.saveUserLocation(location)) - await send(.moveLocation(location)) - } else { + do { + if let location = try await locationClient.requestUserLocation() { + await send(.saveUserLocation(location)) + await send(.moveLocation(location)) + } else { + if isCurrentButtonTap { + await send(.presentAlert(type: .locationPermission)) + await send(.currentButtonTapped(false)) + } + } + } catch { if isCurrentButtonTap { await send(.presentAlert(type: .locationPermission)) await send(.currentButtonTapped(false)) } } } }
🧹 Nitpick comments (4)
Projects/Client/Location/Sources/Client/LocationClient+Endpoint.swift (1)
11-13: Consider removing the unusedLocationEndpointstruct.The
LocationEndpointstruct is empty and has no references anywhere in the codebase. Removing it will reduce code noise.Projects/Client/Location/Sources/Client/LocationClient+Live.swift (1)
14-16: Consider reusing a single LocationService instance to avoid repeated CLLocationManager allocation.Creating a new
LocationService()instance on each call allocates a newCLLocationManagereach time. While the current implementation is functionally correct—the@MainActorisolation and continuation management are properly handled—this pattern is inefficient for repeated location requests. The lazylocationManagerproperty is initialized once per instance, so multiple requests create multiple managers unnecessarily.Consider storing a single shared instance (e.g.,
static let shared = LocationService()) to reuse the sameCLLocationManageracross requests, reducing resource overhead.Projects/Client/Location/Sources/Client/LocationService.swift (2)
9-9: Unused import.
UIKitis imported but not used in this file. OnlyCoreLocationandSharedare needed.🔎 Proposed fix
-import UIKit import CoreLocation import Shared
1-7: File header comment inconsistency.The file header still references "Utility" module (line 3), but this file is now in the
Client/Locationmodule.🔎 Proposed fix
// // LocationService.swift -// Utility +// LocationClient // // Created by Jiyeon on 3/16/25. // Copyright © 2025 com.yongin.pida. All rights reserved. //
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Projects/Client/Location/Project.swiftProjects/Client/Location/Sources/Client/LocationClient+Endpoint.swiftProjects/Client/Location/Sources/Client/LocationClient+Live.swiftProjects/Client/Location/Sources/Client/LocationClient.swiftProjects/Client/Location/Sources/Client/LocationService.swiftProjects/Client/Location/Sources/DTO/Request/dummy1.swiftProjects/Client/Location/Sources/DTO/Response/dummy.swiftProjects/Client/Location/Sources/Entity/entityDummy.swiftProjects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swiftProjects/Feature/Map/Project.swiftProjects/Shared/Sources/Services/LocationService.swiftTuist/Package.swiftTuist/ProjectDescriptionHelpers/TargetDependency/DependencyComponent.swiftTuist/ProjectDescriptionHelpers/TargetDependency/Modules.swiftTuist/Templates/Client/Client.swiftTuist/Templates/Client/client_live.stencil
💤 Files with no reviewable changes (1)
- Projects/Shared/Sources/Services/LocationService.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Projects/Client/Location/Sources/Client/LocationClient.swift (2)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
requestUserLocation(33-48)Projects/Shared/Sources/Services/LocationService.swift (4)
requestUserLocation(34-51)shared(13-88)locationManager(55-72)locationManagerDidChangeAuthorization(74-87)
Projects/Client/Location/Sources/Client/LocationClient+Live.swift (2)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
requestUserLocation(33-48)Projects/Shared/Sources/Services/LocationService.swift (4)
requestUserLocation(34-51)shared(13-88)locationManager(55-72)locationManagerDidChangeAuthorization(74-87)
Projects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift (1)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
requestUserLocation(33-48)
🔇 Additional comments (13)
Projects/Client/Location/Sources/Entity/entityDummy.swift (1)
1-7: Dismiss naming consistency concern — no Entity template exists in Tuist.No Entity template was found in Tuist/Templates/ to establish naming conventions for entity files. The file entityDummy.swift appears to be a standalone file, not template-generated. The review comment's premise that "Tuist template renames Entity files to 'entity_dummy.swift'" cannot be verified against the codebase.
Likely an incorrect or invalid review comment.
Tuist/Templates/Client/client_live.stencil (1)
5-5: No action needed—{{author}} is properly configured as a required template attribute.The
{{author}}variable is defined as a required attribute in the Tuist template configuration (Tuist/Templates/Client/Client.swiftandTuist/Templates/Feature/Feature.swift). Tuist enforces this requirement at render time, ensuring the variable is always provided when generating files. The change from hardcoded author to the template variable is correct and consistent with all other Client and Feature templates in the project.Projects/Client/Location/Sources/DTO/Response/dummy.swift (1)
1-7: Placeholder file for module scaffolding.This dummy file establishes the DTO/Response directory structure for the Location client module and will be replaced with actual response DTOs as needed.
Tuist/Templates/Client/Client.swift (2)
25-25: Path fix correctly addresses the extra directory depth issue.By removing the redundant
/\(name)segment, the generated path now correctly expands toProjects/Client/\(name)/Project.swiftinstead of the previousProjects/Client/\(name)/\(name)/Project.swift. This aligns with the fix described in the PR objectives.
41-51: Improved dummy file naming convention.Renaming dummy.swift to request_dummy.swift, response_dummy.swift, and entity_dummy.swift provides better clarity about the purpose of each placeholder file. The consistent use of client_dummy.stencil as the template is also appropriate.
Projects/Client/Location/Project.swift (1)
10-16: Well-structured Location client project manifest.The static library configuration with dependencies on Client.API and TCA is appropriate for the LocationClient module. This enables proper dependency injection and integration with the Map feature.
Projects/Feature/Map/Project.swift (1)
17-17: Appropriate dependency addition for LocationClient integration.Adding .Client.Location to the Map feature's dependencies correctly enables the refactored LocationClient usage, replacing the previous LocationService singleton pattern. This aligns with the PR's objective to modularize location services.
Tuist/Package.swift (1)
33-33: swift-syntax version 601.0.1 is valid, available, and secure.Version 601.0.1 is a published release (April 2, 2025) available in the Swift Package Registry and has no known security vulnerabilities or CVEs.
Tuist/ProjectDescriptionHelpers/TargetDependency/Modules.swift (1)
37-37: LGTM! Module enum correctly extended.The addition of
Locationto theClientenum is consistent with the existing module pattern and properly wires the new Location client into the dependency system.Tuist/ProjectDescriptionHelpers/TargetDependency/DependencyComponent.swift (1)
31-31: LGTM! Dependency properly exposed.The
Locationdependency is correctly added toTargetDependency.Clientfollowing the same pattern as other client dependencies, enabling proper module dependency resolution.Projects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift (2)
16-16: LGTM on the dependency injection setup.The import and dependency injection via
@Dependency(\.locationClient)follows the standard TCA pattern correctly.Also applies to: 22-22
91-113: Good removal of hardcoded region parameter.The
GetFlowerSpotQuerynow uses dynamic bounds instead of a hardcoded region value, which aligns with the PR objective to remove the hardcoded "SEOUL" region during flower path search.Projects/Client/Location/Sources/Client/LocationService.swift (1)
57-72: Delegate methods and concurrency handling look correct.The
nonisolateddelegate methods correctly dispatch back to@MainActorusingTask { @MainActor in }to safely access the actor-isolated properties (locationManager,continuation). The pattern of stopping updates before finishing indidUpdateLocationsis appropriate.Also applies to: 74-85
| } | ||
|
|
||
| public extension DependencyValues { | ||
| var locationClient : LocationClient { |
There was a problem hiding this comment.
Fix spacing in type annotation.
Swift convention requires no space before the colon in type annotations.
🔎 Proposed fix
- var locationClient : LocationClient {
+ var locationClient: LocationClient {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var locationClient : LocationClient { | |
| var locationClient: LocationClient { |
🤖 Prompt for AI Agents
In Projects/Client/Location/Sources/Client/LocationClient.swift around line 18,
the type annotation has an extra space before the colon; remove the space so the
declaration uses Swift convention (e.g., change "var locationClient :
LocationClient" to "var locationClient: LocationClient") and run a quick grep to
ensure no other declarations in the file use the same incorrect spacing.
| return manager | ||
| }() | ||
|
|
||
| private var continuation: CheckedContinuation<Coordinate?, Never>? |
There was a problem hiding this comment.
Potential continuation leak if called concurrently.
If requestUserLocation() is called while a previous request is still in-flight, the stored continuation is overwritten without resuming the old one. This causes a continuation leak, which Swift runtime treats as a fatal error.
🔎 Proposed fix to guard against concurrent calls
func requestUserLocation() async -> Coordinate? {
+ // Cancel any pending request to prevent continuation leak
+ if continuation != nil {
+ continuation?.resume(returning: nil)
+ continuation = nil
+ }
+
return await withCheckedContinuation { continuation in
self.continuation = continuation
let status = locationManager.authorizationStatus
switch status {
case .authorizedAlways, .authorizedWhenInUse:
locationManager.startUpdatingLocation()
case .notDetermined:
locationManager.requestWhenInUseAuthorization()
case .denied, .restricted:
finish(nil)
default:
finish(nil)
}
}
}Alternatively, consider returning early if a request is already in progress:
func requestUserLocation() async -> Coordinate? {
+ guard continuation == nil else {
+ // Request already in progress
+ return nil
+ }
+
return await withCheckedContinuation { continuation in
// ...
}
}Also applies to: 33-48
🤖 Prompt for AI Agents
In Projects/Client/Location/Sources/Client/LocationService.swift around lines 24
and 33-48, the stored CheckedContinuation can be overwritten when
requestUserLocation() is called concurrently, leaking the previous continuation
and causing a runtime fatal error; fix this by guarding against concurrent
requests: check if continuation != nil at the start of requestUserLocation() and
either return early (no-op) or immediately resume the existing continuation
(e.g., resume(returning: nil) or return an error) before assigning a new one,
ensure any continuation is resumed exactly once and set to nil after resumption,
and consider synchronizing access (actor/private serial queue/lock) to make
these checks and assignments atomic.
| // | ||
| // dummy.swift | ||
| // dummy | ||
| // | ||
| // Created by Jiyeon | ||
| // Copyright © com.pida.me. All rights reserved. | ||
| // |
There was a problem hiding this comment.
Correct the filename in the file header.
Line 2 declares the filename as "dummy.swift" but the actual filename is "dummy1.swift". Update the header to match the actual filename.
🔎 Proposed fix
//
- // dummy.swift
+ // dummy1.swift
// dummy
//
// Created by Jiyeon
// Copyright © com.pida.me. All rights reserved.
//🤖 Prompt for AI Agents
In Projects/Client/Location/Sources/DTO/Request/dummy1.swift around lines 1 to
7, the file header's filename (line 2) is incorrect: it currently reads
"dummy.swift" but the actual file name is "dummy1.swift"; update the second line
of the comment block to "dummy1.swift" so the header matches the real filename
and save the file.
| // | ||
| // dummy.swift | ||
| // dummy | ||
| // | ||
| // Created by Jiyeon | ||
| // Copyright © com.pida.me. All rights reserved. | ||
| // |
There was a problem hiding this comment.
Correct the filename in the file header.
Line 2 declares the filename as "dummy.swift" but the actual filename is "entityDummy.swift". Update the header to match the actual filename.
🔎 Proposed fix
//
- // dummy.swift
+ // entityDummy.swift
// dummy
//
// Created by Jiyeon
// Copyright © com.pida.me. All rights reserved.
//📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // | |
| // dummy.swift | |
| // dummy | |
| // | |
| // Created by Jiyeon | |
| // Copyright © com.pida.me. All rights reserved. | |
| // | |
| // | |
| // entityDummy.swift | |
| // dummy | |
| // | |
| // Created by Jiyeon | |
| // Copyright © com.pida.me. All rights reserved. | |
| // |
🤖 Prompt for AI Agents
In Projects/Client/Location/Sources/Entity/entityDummy.swift around lines 1 to
7, the file header comment lists the filename as "dummy.swift" which does not
match the actual filename "entityDummy.swift"; update the header's filename line
(and any other occurrences in the top comment block) to "entityDummy.swift" so
the file header accurately reflects the real filename.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
9-9: Remove unnecessary UIKit import.The
UIKitimport doesn't appear to be used in this file. OnlyCoreLocationandSharedare needed.🔎 Proposed fix
-import UIKit import CoreLocation import Shared
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Projects/Client/Location/Sources/Client/LocationClient.swiftProjects/Client/Location/Sources/Client/LocationService.swiftProjects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Projects/Client/Location/Sources/Client/LocationClient.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
Projects/Shared/Sources/Services/LocationService.swift (4)
requestUserLocation(34-51)locationManagerDidChangeAuthorization(74-87)shared(13-88)locationManager(55-72)
Projects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift (1)
Projects/Client/Location/Sources/Client/LocationService.swift (1)
requestUserLocation(33-52)
🔇 Additional comments (6)
Projects/Feature/Map/MapFeature/Sources/MapView/LocationFeature.swift (3)
16-16: LGTM! Dependency injection setup is correct.The LocationClient import and dependency injection via
@Dependency(\.locationClient)properly decouples the feature from the concrete location service implementation, improving testability.Also applies to: 22-22
77-89: LGTM! Error handling improves user experience.The refactored location request properly uses the injected
locationClientand adds appropriate error handling. When location retrieval fails and the current button was tapped, the code presents a permission alert and resets the button state, providing clear feedback to users.
91-113: LGTM! Hardcoded region removal enables dynamic queries.The removal of the hardcoded
region: "SEOUL"parameter fromGetFlowerSpotQueryallows the search to properly use dynamic map bounds, making the flower spot queries more accurate and flexible based on the user's actual location.Projects/Client/Location/Sources/Client/LocationService.swift (3)
33-52: LGTM! Continuation guard correctly prevents leak.The guard at lines 35-38 properly addresses the continuation leak concern from the previous review. When a request is already in-flight (
self.continuation != nil), the new caller receivesnilimmediately without overwriting the stored continuation. Since the class is@MainActor, all access is serialized, eliminating race conditions.
54-57: LGTM! Helper method ensures proper cleanup.The
finish()helper centralizes continuation resume logic and ensures the continuation is always set tonilafter resumption, preventing potential double-resume issues.
61-89: LGTM! Delegate methods correctly handle async context.The
nonisolateddelegate methods properly bounce back to@MainActorcontext usingTask { @MainActor in }, ensuring thread-safe access to the continuation. The authorization change handling correctly starts location updates when authorized or completes withnilotherwise.
usa4060
left a comment
usa4060
left a comment
There was a problem hiding this comment.
코멘트 하나 남겼어요~!
이것만 확인하면 될 듯 합니다
| // MARK: - Location Method | ||
| func requestUserLocation() async -> Coordinate? { | ||
| return await withCheckedContinuation { continuation in | ||
| self.continuation = continuation |
There was a problem hiding this comment.
requestUserLocation()가 빠른 시간에 여러번 호출되면, 이전 continuation이 resume 되지 않은 상태로 계속 메모리에 남아있게 될 것 같아요.
self.continuation = continuation 해당 코드 전에 guard 처리 필요할 것 같아요
if continuation != nil {
continuation?.resume(returning: nil)
continuation = nil
}이런 식으루
#️⃣ Issue Number
🔎 작업 내용
/\(name)가 포함되어있어, depth가 하나 더 생기는 문제가 있었음.📷 스크린샷
경기 데이터 검색 성공~
🛠️ 기타 공유 내용
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.