-
Notifications
You must be signed in to change notification settings - Fork 0
Replace Combine with AsyncSequence and @Observable #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
00fd426
9a479ea
776dc7f
8e5f35c
798dbfd
196facb
00715f7
50c0687
aec2002
89948e3
36403f9
c8225fd
9b879f8
03eecb6
0475f4b
0c63d0f
cefcbf3
1ae45c6
78a5462
d2146fe
97d2def
dbc2667
69be639
91c7804
f5de172
4f21e82
9ae20b5
60b9eac
2991460
18ba6ab
d0cd4f2
951f3cc
0c2984f
5b3a2bc
065a4e6
234dca8
23e7097
f9d333e
610e836
a440ee7
b0cd7a4
b0817a6
e2f81ed
7fcc3d8
c7a8a94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,10 +14,11 @@ Review all recent code changes thoroughly and provide a structured, actionable a | |
|
|
||
| ## Project Context | ||
|
|
||
| - **Branch**: feature/navigation-stack — Pure SwiftUI, NavigationPath, single AppCoordinator (ObservableObject), Combine | ||
| - **Branch**: feature/async-sequence — Pure SwiftUI, @Observable, AsyncSequence + StreamBroadcaster, zero Combine | ||
| - **Packages**: `FunCore` → `FunModel` → `FunViewModel` / `FunServices` → `FunUI` → `FunCoordinator` | ||
| - **Dependency direction**: Never import upward. ViewModel must NOT import UI or Coordinator. | ||
| - **UIKit**: Zero UIKit in this branch — flag any `import UIKit` as a critical issue | ||
| - **Combine**: Zero Combine in this branch — flag any `import Combine` as critical | ||
| - **DI**: ServiceLocator with `@Service` property wrapper, session-scoped (LoginSession / AuthenticatedSession) | ||
| - **Testing**: Swift Testing framework, mocks in FunModelTestSupport | ||
| - **Lint**: SwiftLint with custom rules (no_print, weak_coordinator_in_viewmodel, no_direct_userdefaults) | ||
|
|
@@ -38,19 +39,23 @@ Review all recent code changes thoroughly and provide a structured, actionable a | |
| ### Step 3: Architecture Check | ||
| - Package dependency direction respected? | ||
| - No `import UIKit` — pure SwiftUI branch | ||
| - No `import Combine` — pure AsyncSequence branch | ||
| - No coordinator references in ViewModels (except weak closures) | ||
| - No `print()` — use LoggerService | ||
| - No `UserDefaults.standard` outside Services | ||
| - Navigation logic only in Coordinators (AppCoordinator) | ||
| - NavigationPath mutations only in coordinator, not in Views | ||
| - Protocols in Core (reusable) or Model (domain), never in Services/ViewModel/UI/Coordinator | ||
| - Reactive pattern: Combine (`@Published`, `@StateObject`, `@ObservedObject`, `.sink`) | ||
| - Reactive pattern: `@Observable`, `AsyncStream`, `StreamBroadcaster`, `for await`, `Task` | ||
| - `@ObservationIgnored` on services and non-UI state | ||
| - `@State` (not `@StateObject`) for owning @Observable objects | ||
|
|
||
| ### Step 4: Correctness Check | ||
|
Comment on lines
46
to
49
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why deleting this general rules?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restored the general memory management wording. Now extends it with async-specific |
||
| - **Logic errors**: Algorithms, conditions, control flow | ||
| - **Type safety**: Force unwraps, force casts, unsafe assumptions | ||
| - **Concurrency**: `@MainActor` isolation, `Sendable` conformance, Swift 6 strict | ||
g-enius marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - **Memory management**: `[weak self]` and `[weak coordinator]` in closures | ||
| - **Memory management**: `[weak self]` and `[weak coordinator]` in closures, `guard let self` inside `for await` loops | ||
| - **Stream lifecycle**: Tasks stored for cancellation? Cleaned up properly? | ||
| - **API contracts**: Public interfaces used correctly | ||
|
|
||
| ### Step 5: Quality Check | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be the same as the navigation-stack branch. If change, should change in the previous PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored. NavigationPath rule still applies — added it back.