Conversation
Contributor
🛠️ 이슈와 PR의 Labels 동기화를 스킵했어요. |
Contributor
✅ PR의 Assign 자동 지정을 성공했어요! |
Contributor
Walkthrough앱 라우팅 시스템을 위한 Changes
Sequence DiagramsequenceDiagram
actor User
participant App as TodayWhatApp
participant URLHandler as onOpenURL Handler
participant RouteStore as TodayWhatAppRouteStore
participant MainView as MainView
participant Core as MainCore
User->>App: Opens URL (todaywhat://meal)
App->>URLHandler: Routes to onOpenURL handler
URLHandler->>URLHandler: Parse URL to TodayWhatAppRoute
URLHandler->>RouteStore: request(route)
RouteStore->>RouteStore: Set pendingRoute
RouteStore-->>MainView: pendingRoutePublisher emits
MainView->>MainView: consumePendingRoute()
MainView->>MainView: apply(route:) → determine tab index
MainView->>Core: Send MainCore.Action.tabTapped(index)
Core->>MainView: Update selection state
MainView-->>User: Display requested screen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements App Intents and Shortcuts for meal and timetable features, enabling users to access this information via Siri and the Shortcuts app. It introduces a centralized routing system through the new AppRouteClient module and adds a pending event queue in TWLog to handle analytics from app extensions using App Groups. The review feedback identifies several critical improvement opportunities, including addressing a type mismatch and potential race conditions in the logging queue, ensuring the uniqueness of identifiers for intent entities, and adhering to dependency injection principles to improve the testability of the new intents.
|
|
||
| static func enqueueEvent(_ eventLog: any EventLog) { | ||
| guard let defaults = appGroupDefaults else { return } | ||
| var queue = defaults.array(forKey: pendingEventsKey) as? [[String: String]] ?? [] |
Contributor
There was a problem hiding this comment.
EventLog의 params는 일반적으로 [String: Any] 타입을 가집니다. 하지만 여기서 queue를 [[String: String]]으로 캐스팅하고 있어, 만약 로그 파라미터에 문자열이 아닌 타입(Int, Bool 등)이 포함되어 있다면 캐스팅에 실패하여 기존 큐 데이터가 무시되거나 컴파일 에러가 발생할 수 있습니다. [[String: Any]] 타입을 사용하도록 수정하는 것이 안전합니다.
Suggested change
| var queue = defaults.array(forKey: pendingEventsKey) as? [[String: String]] ?? [] | |
| var queue = defaults.array(forKey: pendingEventsKey) as? [[String: Any]] ?? [] |
Comment on lines
+132
to
+139
| static func enqueueEvent(_ eventLog: any EventLog) { | ||
| guard let defaults = appGroupDefaults else { return } | ||
| var queue = defaults.array(forKey: pendingEventsKey) as? [[String: String]] ?? [] | ||
| var entry = eventLog.params | ||
| entry["__event_name"] = eventLog.name | ||
| queue.append(entry) | ||
| defaults.set(queue, forKey: pendingEventsKey) | ||
| } |
Contributor
There was a problem hiding this comment.
App Group의 UserDefaults를 사용하여 여러 프로세스(App, Intent Extension 등)에서 동시에 enqueueEvent를 호출할 경우, 읽기-수정-쓰기(read-modify-write) 과정이 원자적(atomic)으로 이루어지지 않아 데이터가 유실되거나 덮어씌워지는 레이스 컨디션이 발생할 수 있습니다. 분석 데이터의 정확성이 중요하다면 파일 기반의 락(File Lock)이나 별도의 원자적 저장 방식을 고려해 보세요.
| } | ||
|
|
||
| init(period: Int, subject: String) { | ||
| self.id = "\(period)-\(subject)" |
Contributor
There was a problem hiding this comment.
교시(period)와 과목명(subject)의 조합만으로는 AppEntity의 ID 고유성을 완벽히 보장하기 어려울 수 있습니다. 예를 들어, 같은 교시에 분반 수업이 있거나 데이터 오류로 중복 항목이 생길 경우 Shortcuts 앱에서 예기치 않은 동작이 발생할 수 있습니다. UUID().uuidString을 사용하거나 더 고유한 식별자를 사용하는 것을 권장합니다.
Suggested change
| self.id = "\(period)-\(subject)" | |
| self.id = UUID().uuidString |
| )) | ||
|
|
||
| let targetDate = daySelection == .specify ? (specifyDate ?? Date()) : daySelection.targetDate | ||
| let meal = try await MealClient.liveValue.fetchMeal(targetDate) |
Contributor
There was a problem hiding this comment.
MealClient.liveValue를 직접 참조하여 사용하는 것은 의존성 주입 원칙에 어긋나며 단위 테스트 작성을 어렵게 만듭니다. 프로젝트 내 다른 Intent에서 사용된 것처럼 @AppDependency를 사용하거나, TCA의 @Dependency를 활용하여 의존성을 주입받는 방식으로 개선하는 것이 좋습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Projects/App/Intents/TodayWhatAppShortcuts.swift (1)
5-115: Shortcut 선언 중복을 헬퍼로 줄이면 유지보수가 쉬워집니다.현재도 동작은 명확하지만, 식사/시간표 Shortcut 패턴이 반복돼 추후 문구 수정 시 누락 위험이 있습니다. 헬퍼 함수로 축약을 고려해도 좋겠습니다.
♻️ 예시 리팩터링 방향
struct TodayWhatAppShortcuts: AppShortcutsProvider { static var appShortcuts: [AppShortcut] { - AppShortcut( - intent: GetMealIntent(mealTime: .all, daySelection: .today), - phrases: [ ... ], - shortTitle: "오늘 급식", - systemImageName: "fork.knife" - ) - ... + mealShortcut(.all, .today, "오늘 급식", "fork.knife", "오늘 급식") + mealShortcut(.breakfast, .today, "오늘 아침 급식", "sunrise", "오늘 아침 급식") + ... } + + private static func mealShortcut( + _ mealTime: MealPartTime, + _ day: DaySelection, + _ title: LocalizedStringResource, + _ image: String, + _ phraseCore: String + ) -> AppShortcut { + AppShortcut( + intent: GetMealIntent(mealTime: mealTime, daySelection: day), + phrases: [ + "\(.applicationName) \(phraseCore)", + "\(phraseCore) \(.applicationName)", + "\(.applicationName)에서 \(phraseCore) 보여줘" + ], + shortTitle: title, + systemImageName: image + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/App/Intents/TodayWhatAppShortcuts.swift` around lines 5 - 115, The appShortcuts getter contains repeated AppShortcut constructions for meals and timetables (using GetMealIntent and GetTimeTableIntent) making maintenance error-prone; refactor by extracting small helper(s) (e.g., makeMealShortcut(mealTime:daySelection:shortTitle:systemImageName:) and makeTimeTableShortcut(daySelection:shortTitle:systemImageName:)) that build the phrases array and return AppShortcut, then replace each repeated AppShortcut call in static var appShortcuts with calls to these helpers to centralize phrase formatting and reduce duplication.Projects/App/Intents/GetMealIntent.swift (2)
143-180:MealDaySelection과TimeTableDaySelection이 중복됩니다.두 enum이 거의 동일한 구조를 가지고 있습니다 (yesterday, today, tomorrow, specify 케이스와
displayName,targetDate프로퍼티). 공통 타입으로 추출하면 유지보수성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/App/Intents/GetMealIntent.swift` around lines 143 - 180, MealDaySelection duplicates the structure and behavior of TimeTableDaySelection; extract a single shared enum (e.g., DaySelection: String, AppEnum) that implements the common cases (.yesterday, .today, .tomorrow, .specify), the displayName and targetDate computed properties, plus the TypeDisplayRepresentation and caseDisplayRepresentations, then replace MealDaySelection and TimeTableDaySelection usages with the new DaySelection (or create lightweight typealiases if distinct names are required) so you keep one implementation for displayName/targetDate logic and update all references to use the shared symbol instead of duplicated enums.
199-202:MealData구조체에@available어노테이션이 누락되었습니다.다른 타입들(
MealDaySelection,MealResultEntity등)은@available(iOS 16, macOS 13, *)을 포함하고 있지만,MealData에는 누락되어 있습니다. 일관성을 위해 추가하는 것이 좋습니다.♻️ 제안하는 수정
+@available(iOS 16, macOS 13, *) struct MealData { let name: String let subMeal: Meal.SubMeal }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Projects/App/Intents/GetMealIntent.swift` around lines 199 - 202, MealData is missing the platform availability annotation used elsewhere; add the same `@available`(iOS 16, macOS 13, *) attribute to the MealData struct so it matches MealDaySelection and MealResultEntity and avoids cross-target runtime issues, i.e., annotate the MealData declaration with `@available`(iOS 16, macOS 13, *) immediately above the struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Projects/App/Intents/GetTimeTableIntent.swift`:
- Around line 159-165: The period indicator Text inside the ForEach over
timeTables in GetTimeTableIntent.swift currently applies .clipShape(Circle())
but no background color; update the modifier chain on Text("\(timeTable.perio)")
(or immediately after .frame(width: 24, height: 24)) to add a circular
background, e.g. use .background(Circle().fill(Color.accentColor)) or
.background(Circle().fill(Color(...))) and set a contrasting
.foregroundColor(...) as needed so the circular indicator is visually distinct.
In `@Projects/App/iOS/Sources/Application/AppDelegate.swift`:
- Line 40: TWLog.flushPendingEvents() is synchronous and can block the main
thread during app launch; change the call so it runs off the main thread (e.g.,
wrap TWLog.flushPendingEvents() in DispatchQueue.global(qos: .background).async
{ ... } or Task.detached { ... } inside AppDelegate startup), ensuring any
returned errors or completion handling are marshalled back to the main thread if
needed; update the code paths that call TWLog.flushPendingEvents() to use this
background dispatch so launch is not delayed.
In `@Projects/App/Project.swift`:
- Line 106: macOS target includes Intents/** but the Siri entitlement is
missing; open the three entitlements (TodayWhat.entitlements,
TodayWhat_Mac_App.entitlements, TodayWhatWidget.entitlements) and add the
com.apple.developer.siri key with a boolean true value, then ensure each
entitlements file is assigned to the correct target (iOS app, macOS app, and iOS
widget) in the project so the Intents code is allowed to expose App Intents to
Shortcuts/Siri.
In `@Projects/Shared/TWLog/Sources/TWLog.swift`:
- Around line 125-129: The appGroupID is hardcoded for iOS/watchOS, causing
macOS to use the wrong suiteName; replace the hardcoded
"group.baegteun.TodayWhat" in TWLog.swift with the canonical AppGroup value from
the shared AppGroup enum (from
Projects/Shared/ConstantUtil/Sources/AppGroup.swift) so the correct
platform-specific identifier is used; import the ConstantUtil module if
necessary and change private static let appGroupID and appGroupDefaults to
obtain the suiteName from AppGroup (e.g., AppGroup.<todayWhatCase>.rawValue or
AppGroup.<todayWhatCase>.identifier) instead of the literal string so macOS
intents and the main app share the same user defaults.
---
Nitpick comments:
In `@Projects/App/Intents/GetMealIntent.swift`:
- Around line 143-180: MealDaySelection duplicates the structure and behavior of
TimeTableDaySelection; extract a single shared enum (e.g., DaySelection: String,
AppEnum) that implements the common cases (.yesterday, .today, .tomorrow,
.specify), the displayName and targetDate computed properties, plus the
TypeDisplayRepresentation and caseDisplayRepresentations, then replace
MealDaySelection and TimeTableDaySelection usages with the new DaySelection (or
create lightweight typealiases if distinct names are required) so you keep one
implementation for displayName/targetDate logic and update all references to use
the shared symbol instead of duplicated enums.
- Around line 199-202: MealData is missing the platform availability annotation
used elsewhere; add the same `@available`(iOS 16, macOS 13, *) attribute to the
MealData struct so it matches MealDaySelection and MealResultEntity and avoids
cross-target runtime issues, i.e., annotate the MealData declaration with
`@available`(iOS 16, macOS 13, *) immediately above the struct.
In `@Projects/App/Intents/TodayWhatAppShortcuts.swift`:
- Around line 5-115: The appShortcuts getter contains repeated AppShortcut
constructions for meals and timetables (using GetMealIntent and
GetTimeTableIntent) making maintenance error-prone; refactor by extracting small
helper(s) (e.g.,
makeMealShortcut(mealTime:daySelection:shortTitle:systemImageName:) and
makeTimeTableShortcut(daySelection:shortTitle:systemImageName:)) that build the
phrases array and return AppShortcut, then replace each repeated AppShortcut
call in static var appShortcuts with calls to these helpers to centralize phrase
formatting and reduce duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 048ba576-c65a-464d-8f8e-bac3f78345cd
📒 Files selected for processing (15)
Plugin/DependencyPlugin/ProjectDescriptionHelpers/ModulePaths.swiftProjects/App/Intents/GetMealIntent.swiftProjects/App/Intents/GetTimeTableIntent.swiftProjects/App/Intents/MealPartTimeSelectionIntent.swiftProjects/App/Intents/TodayWhatAppOpenIntent.swiftProjects/App/Intents/TodayWhatAppShortcuts.swiftProjects/App/Project.swiftProjects/App/iOS/Sources/Application/AppDelegate.swiftProjects/App/iOS/Sources/Application/TodayWhatApp.swiftProjects/App/iOS/Support/Info.plistProjects/Feature/MainFeature/Project.swiftProjects/Feature/MainFeature/Sources/MainView.swiftProjects/Shared/AppRouteClient/Project.swiftProjects/Shared/AppRouteClient/Sources/TodayWhatAppRoute.swiftProjects/Shared/TWLog/Sources/TWLog.swift
Comment on lines
+159
to
+165
| ForEach(timeTables, id: \.self) { timeTable in | ||
| HStack(spacing: 10) { | ||
| Text("\(timeTable.perio)") | ||
| .font(.caption) | ||
| .fontWeight(.bold) | ||
| .frame(width: 24, height: 24) | ||
| .clipShape(Circle()) |
Contributor
There was a problem hiding this comment.
교시 인디케이터에 배경색이 누락되었습니다.
.clipShape(Circle())만 적용되어 있고 배경색이 없어서 원형 인디케이터가 시각적으로 구분되지 않습니다.
🎨 배경색 추가 제안
Text("\(timeTable.perio)")
.font(.caption)
.fontWeight(.bold)
.frame(width: 24, height: 24)
+ .background(Color.gray.opacity(0.2))
.clipShape(Circle())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/App/Intents/GetTimeTableIntent.swift` around lines 159 - 165, The
period indicator Text inside the ForEach over timeTables in
GetTimeTableIntent.swift currently applies .clipShape(Circle()) but no
background color; update the modifier chain on Text("\(timeTable.perio)") (or
immediately after .frame(width: 24, height: 24)) to add a circular background,
e.g. use .background(Circle().fill(Color.accentColor)) or
.background(Circle().fill(Color(...))) and set a contrasting
.foregroundColor(...) as needed so the circular indicator is visually distinct.
| } | ||
| DesignSystemFontFamily.Suit.all.forEach { $0.register() } | ||
| initializeAnalyticsUserID() | ||
| TWLog.flushPendingEvents() |
Contributor
There was a problem hiding this comment.
앱 시작 경로에서 동기 플러시 호출로 메인 스레드가 막힐 수 있습니다.
Line 40에서 직접 TWLog.flushPendingEvents()를 호출하면, Projects/Shared/TWLog/Sources/TWLog.swift Line 141-151/Line 110-115의 동기 처리(저장소 접근 + 이벤트 전송) 때문에 런치 지연 리스크가 큽니다. 백그라운드로 분리하는 편이 안전합니다.
🔧 제안 수정안
- TWLog.flushPendingEvents()
+ DispatchQueue.global(qos: .utility).async {
+ TWLog.flushPendingEvents()
+ }📝 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.
Suggested change
| TWLog.flushPendingEvents() | |
| DispatchQueue.global(qos: .utility).async { | |
| TWLog.flushPendingEvents() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/App/iOS/Sources/Application/AppDelegate.swift` at line 40,
TWLog.flushPendingEvents() is synchronous and can block the main thread during
app launch; change the call so it runs off the main thread (e.g., wrap
TWLog.flushPendingEvents() in DispatchQueue.global(qos: .background).async { ...
} or Task.detached { ... } inside AppDelegate startup), ensuring any returned
errors or completion handling are marshalled back to the main thread if needed;
update the code paths that call TWLog.flushPendingEvents() to use this
background dispatch so launch is not delayed.
| deploymentTargets: .macOS("12.0"), | ||
| infoPlist: .file(path: "macOS/Support/Info.plist"), | ||
| sources: ["macOS/Sources/**"], | ||
| sources: ["macOS/Sources/**", "Intents/**"], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
"Projects/App/iOS/Support/TodayWhat.entitlements" \
"Projects/App/macOS/Support/TodayWhat_Mac_App.entitlements" \
"Projects/App/iOS-Widget/Support/TodayWhatWidget.entitlements"
do
echo "== $f =="
if [ -f "$f" ]; then
rg -n "com.apple.developer.siri" "$f" || true
else
echo "MISSING: $f"
fi
doneRepository: todaywhat/TodayWhat-iOS
Length of output: 251
Siri 엔타이틀먼트 키 추가 필요
Line 106에서 macOS 타깃이 Intents/**를 컴파일 대상으로 포함하고 있으나, 다음 엔타이틀먼트 파일들에서 com.apple.developer.siri 키가 누락되어 있습니다:
- Projects/App/iOS/Support/TodayWhat.entitlements
- Projects/App/macOS/Support/TodayWhat_Mac_App.entitlements
- Projects/App/iOS-Widget/Support/TodayWhatWidget.entitlements
App Intents가 Shortcuts 및 Siri에 제대로 노출되려면 해당 엔타이틀먼트 키를 추가해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/App/Project.swift` at line 106, macOS target includes Intents/** but
the Siri entitlement is missing; open the three entitlements
(TodayWhat.entitlements, TodayWhat_Mac_App.entitlements,
TodayWhatWidget.entitlements) and add the com.apple.developer.siri key with a
boolean true value, then ensure each entitlements file is assigned to the
correct target (iOS app, macOS app, and iOS widget) in the project so the
Intents code is allowed to expose App Intents to Shortcuts/Siri.
Comment on lines
+125
to
+129
| private static let appGroupID = "group.baegteun.TodayWhat" | ||
| private static let pendingEventsKey = "tw_pending_analytics_events" | ||
|
|
||
| private static var appGroupDefaults: UserDefaults? { | ||
| UserDefaults(suiteName: appGroupID) |
Contributor
There was a problem hiding this comment.
macOS에서 App Group ID 불일치로 인해 이벤트 큐가 공유되지 않습니다.
하드코딩된 "group.baegteun.TodayWhat" 값은 iOS/watchOS에서만 유효합니다. Projects/Shared/ConstantUtil/Sources/AppGroup.swift에 따르면 macOS에서는 "235C2RVZ7L.TodayWhat"을 사용합니다. 이 PR은 macOS 13 지원을 추가하므로, macOS Intent에서 enqueue된 이벤트가 메인 앱에서 flush되지 않습니다.
기존 AppGroup enum을 재사용하는 것을 권장합니다.
🔧 제안하는 수정 방안
+import ConstantUtil
+
// MARK: - Pending Event Queue (for AppIntent / Extension)
public extension TWLog {
- private static let appGroupID = "group.baegteun.TodayWhat"
+ private static let appGroupID = AppGroup.group.rawValue
private static let pendingEventsKey = "tw_pending_analytics_events"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Projects/Shared/TWLog/Sources/TWLog.swift` around lines 125 - 129, The
appGroupID is hardcoded for iOS/watchOS, causing macOS to use the wrong
suiteName; replace the hardcoded "group.baegteun.TodayWhat" in TWLog.swift with
the canonical AppGroup value from the shared AppGroup enum (from
Projects/Shared/ConstantUtil/Sources/AppGroup.swift) so the correct
platform-specific identifier is used; import the ConstantUtil module if
necessary and change private static let appGroupID and appGroupDefaults to
obtain the suiteName from AppGroup (e.g., AppGroup.<todayWhatCase>.rawValue or
AppGroup.<todayWhatCase>.identifier) instead of the literal string so macOS
intents and the main app share the same user defaults.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 개요
📃 작업내용
🔀 변경사항
🙋♂️ 질문사항
🍴 사용방법
🤔 고민과 해결방법
🎸 기타
Summary by CodeRabbit
릴리즈 노트
새로운 기능
버전