Repository navigation
Conversation
- destinationView 내부 임시 View 생성 코드 추가
Owner
Author
|
@sourcery-ai review |
Reviewer's Guide독립적인 내비게이션 스택에 연결된 탭 기반 컨테이너를 사용하는 새로운 메인 MohaView를 구현하고, 공통 라우팅 추상화를 도입하며, 이를 앱의 진입점으로 설정합니다. File-Level Changes
Tips and commandsSourcery와 상호작용하기
환경 맞춤 설정대시보드에 접속하여:
도움 받기Original review guide in EnglishReviewer's GuideImplements a new main MohaView with a tab-based container wired to independent navigation stacks per tab, introduces a shared routing abstraction, and makes it the app’s entry point. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 이 PR에서 이슈 한 가지를 발견했고, 전반적인 피드백을 몇 가지 남깁니다:
NavigationRouter의 API가 연관 타입Destination대신any Route를 받도록 되어 있어서, 다른 탭의 라우트를 실수로 푸시하는 문제가 생길 수 있습니다. 각 라우터가 타입 안전성을 유지하고 서로 분리되도록, 이 메서드들을Destination으로 제한하는 것을 고려해 주세요.Route프로토콜이 문자열id와 커스텀Hashable/Equatable구현을 강제하고 있는데, 실제로는 enum이 합성된(con synthesized) 적합성으로 충분히 처리할 수 있습니다. 타입을 넘나드는 string 키가 꼭 필요하지 않다면, enum의 기본Hashable을 사용하도록 단순화하거나id를 더 강하게 타입 지정하면 보일러플레이트를 줄이고 잠재적인 ID 충돌도 줄일 수 있습니다.MainRouter가 현재MohaView.TabItem에 의존하고 있어, 핵심 라우팅 객체가 특정 뷰에 묶여 있습니다.TabItem을 별도의 타입(또는 모듈)로 분리하면 내비게이션 상태를 UI 레이어에서 분리할 수 있고, 라우터를 더 쉽게 재사용할 수 있습니다.
AI 에이전트를 위한 프롬프트
Please address the comments from this code review:
## Overall Comments
- `NavigationRouter`’s APIs accept `any Route` instead of the `Destination` associated type, which allows accidentally pushing routes from other tabs; consider constraining these methods to `Destination` to keep each router type-safe and isolated.
- The `Route` protocol forces a string `id` and custom `Hashable`/`Equatable` while the enums could use synthesized conformance; if you don’t need a cross-type string key, simplifying to the enum’s default `Hashable` or making `id` more strongly typed would reduce boilerplate and potential ID collisions.
- `MainRouter` currently depends on `MohaView.TabItem`, tying a core routing object to a specific view; extracting `TabItem` to a standalone type (or module) would decouple navigation state from the UI layer and make the router easier to reuse.
## Individual Comments
### Comment 1
<location path="Moha/Moha/Sources/Navigation/Router/NavigationRouter.swift" line_range="10-13" />
<code_context>
+
+import SwiftUI
+
+protocol NavigationRouter: AnyObject {
+ associatedtype Destination: Route
+
+ var path: NavigationPath { get set }
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `Destination` 연관 타입이 정의되어 있지만 라우터 API에서 강제되지 않아, 라우터 간 라우트가 잘못 사용될 수 있습니다.
`NavigationRouter`는 `associatedtype Destination: Route`를 정의하고 있지만, `push(to:)`와 `replace(with:)` 같은 라우터 API는 여전히 `any Route`를 받습니다. 이 때문에 다른 라우터의 라우트를 전달할 수 있고(예: `ReportRoute`를 `HomeRouter`에 전달) 런타임에서야 실패하게 됩니다. 이런 API를 `Destination`을 사용하도록 변경하면(예: `func push(to route: Destination)`) 컴파일 타임에 타입 안전성을 보장하고, 연관 타입 설계와도 더 잘 맞게 됩니다.
Suggested implementation:
```
extension NavigationRouter {
func push(to route: Destination) {
```
라우터 전체 API에 이 제안을 완전히 적용하려면 다음을 수행해야 합니다:
1. 현재 `any Route`를 받는 다른 모든 라우터 메서드들(예: `replace(with:)`, `insert(_:)` 등)을 `Destination`을 받도록 업데이트합니다.
2. 라우트를 동적으로 생성하는 오버로드나 헬퍼가 있다면, 다음 중 하나를 만족하도록 합니다:
- 필요한 곳에서는 `Destination`을 제네릭으로 사용하거나,
- `any Route` 대신 라우터의 `Destination` 타입으로 제한합니다.
3. 여전히 `any Route`를 전달하는 호출부가 있다면, 컴파일러가 올바른 라우트 사용을 강제할 수 있도록 각 라우터의 `Destination` 타입을 사용하도록 업데이트합니다.
</issue_to_address>제가 더 유용해질 수 있도록 도와주세요! 각 댓글에 👍 또는 👎를 눌러 주시면, 그 피드백을 바탕으로 리뷰 품질을 개선하겠습니다.
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
NavigationRouter’s APIs acceptany Routeinstead of theDestinationassociated type, which allows accidentally pushing routes from other tabs; consider constraining these methods toDestinationto keep each router type-safe and isolated.- The
Routeprotocol forces a stringidand customHashable/Equatablewhile the enums could use synthesized conformance; if you don’t need a cross-type string key, simplifying to the enum’s defaultHashableor makingidmore strongly typed would reduce boilerplate and potential ID collisions. MainRoutercurrently depends onMohaView.TabItem, tying a core routing object to a specific view; extractingTabItemto a standalone type (or module) would decouple navigation state from the UI layer and make the router easier to reuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `NavigationRouter`’s APIs accept `any Route` instead of the `Destination` associated type, which allows accidentally pushing routes from other tabs; consider constraining these methods to `Destination` to keep each router type-safe and isolated.
- The `Route` protocol forces a string `id` and custom `Hashable`/`Equatable` while the enums could use synthesized conformance; if you don’t need a cross-type string key, simplifying to the enum’s default `Hashable` or making `id` more strongly typed would reduce boilerplate and potential ID collisions.
- `MainRouter` currently depends on `MohaView.TabItem`, tying a core routing object to a specific view; extracting `TabItem` to a standalone type (or module) would decouple navigation state from the UI layer and make the router easier to reuse.
## Individual Comments
### Comment 1
<location path="Moha/Moha/Sources/Navigation/Router/NavigationRouter.swift" line_range="10-13" />
<code_context>
+
+import SwiftUI
+
+protocol NavigationRouter: AnyObject {
+ associatedtype Destination: Route
+
+ var path: NavigationPath { get set }
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `Destination` associated type is defined but not enforced in the router API, which can lead to cross-router route misuse.
`NavigationRouter` defines `associatedtype Destination: Route`, but router APIs like `push(to:)` and `replace(with:)` still take `any Route`. This allows passing routes from other routers (e.g. `ReportRoute` into `HomeRouter`) and only failing at runtime. Updating these APIs to use `Destination` (e.g. `func push(to route: Destination)`) would enforce compile-time type safety and align with the associated type design.
Suggested implementation:
```
extension NavigationRouter {
func push(to route: Destination) {
```
To fully implement the suggestion across the router API, you should:
1. Update all other router methods that currently accept `any Route` (e.g. `replace(with:)`, `insert(_:)`, etc.) to instead accept `Destination`.
2. If there are overloads or helpers that construct routes dynamically, ensure they either:
- Are generic over `Destination` where appropriate, or
- Are constrained to the router’s `Destination` type instead of `any Route`.
3. If any call sites still pass `any Route`, update them to use the specific router’s `Destination` type so the compiler can enforce the correct route usage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- 프로젝트 경로 수정 - 시뮬레이터 수정 - 누락 기호 추가 ( ' \. ')
- any Route -> Destination 수정으로 인해 함수 동작 오류 방지
- 해당값이 없어도 enum은 Hashable 채택 가능 - Router에서의 associatedType을 제한하기 위해 Route 프로토콜은 삭제하지 않음
Owner
Author
-> associatedType 수정 완료
->
-> |
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.
🎫 관련 이슈 (Linked Issues)
Closes #2
🛠 작업 내용 (What I did)
MohaView에 Home, Report, Settings 탭을 제공하는 기본TabView컨테이너를 구현했습니다.NavigationStack과 Router를 연결했습니다.NavigationRouter프로토콜로 정의하고 탭별 Route 타입을 구성했습니다.MainRouter에서 선택된 탭과 탭별 Router를 관리하고 Environment로 주입했습니다.💻 구현 상세 (Implementation Details)
TabView(selection:)과 각 탭의tag를 연결하여 선택 상태를 명시적으로 관리합니다.NavigationPath를 사용하도록 구성해 탭 전환 시 탐색 상태가 섞이지 않도록 했습니다.push,pop,reset,popTo,replace를 공통 Router 동작으로 제공했습니다.navigationDestination에서 각 Route에 대응하는 화면을 생성하도록 분리했습니다.📸 스크린샷 (Screenshots)
✅ 검증 (Verification)
🧐 리뷰 포인트 (Review Points)
MainRouter가 탭 선택 상태와 탭별 Router를 소유하는 구조가 적절한지 확인 부탁드립니다.NavigationPath분리와 Environment 주입 방식이 이후 화면 확장에 적합한지 봐주세요.NavigationRouter의 공통 탐색 동작과 Route 타입 구성이 명확한지 확인 부탁드립니다.