Fix/#162 2차 QA 반영 #167
Open
Fix/#162 2차 QA 반영 #167
+170
−124
Conversation
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
y-eonee
requested review from
a team,
soseoyo12,
sum130 and
wotjs020708
and removed request for
a team
Open
1 task
📝 WalkthroughWalkthrough버전을 1.0에서 1.1로 업데이트하고, 전역 CherryLevel enum 및 관련 게이지바 에셋을 추가했으며 캘린더·챌린지·스플래시 뷰 레이아웃과 ScrollTopPreferenceKey 동작을 변경했습니다. SplashView 네비게이션은 항상 온보딩으로 이동하도록 단순화되었습니다. Changes
Sequence Diagram(s)(해당 변경들은 주로 UI 레이아웃, enum/에셋 추가 및 단일 컴포넌트 동작 변경이므로 시퀀스 다이어그램을 생략합니다.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/View/ChallengeLoadingView.swift`:
- Around line 36-41: The top padding on TypographyText(".padding(.top,
17.adjustedH)") and the following Spacer() likely duplicate spacing; pick one
approach and make it explicit in ChallengeLoadingView: either remove the
.padding(.top, 17.adjustedH) and rely on Spacer() to push content, or replace
Spacer() with Spacer().frame(height: 17.adjustedH) (or a fixed Spacer height) to
represent the intended fixed gap; update TypographyText, padding, and Spacer
usage accordingly so only one mechanism controls the vertical space.
Comment on lines
+36
to
+41
| TypographyText("잠시만 기다려주세요!", style: .title2_m_16, color: .gray800) | ||
| .frame(height: 24.adjustedH) | ||
| .padding(.top, 17.adjustedH) | ||
|
|
||
| Spacer() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
중복 가능성이 있는 여백 설정
Line 38의 .padding(.top, 17.adjustedH)와 Line 40-41의 Spacer()가 함께 사용되고 있습니다. Spacer()가 남은 공간을 채우므로, 상단 패딩이 의도한 레이아웃인지 확인해 주세요. 만약 고정된 간격이 필요하다면 패딩 대신 Spacer().frame(height:)를 사용하는 것이 더 명확할 수 있습니다.
♻️ 제안된 대안
TypographyText("잠시만 기다려주세요!", style: .title2_m_16, color: .gray800)
.frame(height: 24.adjustedH)
- .padding(.top, 17.adjustedH)
Spacer()
+ .frame(minHeight: 17.adjustedH)📝 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
| TypographyText("잠시만 기다려주세요!", style: .title2_m_16, color: .gray800) | |
| .frame(height: 24.adjustedH) | |
| .padding(.top, 17.adjustedH) | |
| Spacer() | |
| TypographyText("잠시만 기다려주세요!", style: .title2_m_16, color: .gray800) | |
| .frame(height: 24.adjustedH) | |
| Spacer() | |
| .frame(minHeight: 17.adjustedH) | |
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/ChallengeView/View/ChallengeLoadingView.swift`
around lines 36 - 41, The top padding on TypographyText(".padding(.top,
17.adjustedH)") and the following Spacer() likely duplicate spacing; pick one
approach and make it explicit in ChallengeLoadingView: either remove the
.padding(.top, 17.adjustedH) and rely on Spacer() to push content, or replace
Spacer() with Spacer().frame(height: 17.adjustedH) (or a fixed Spacer height) to
represent the intended fixed gap; update TypographyText, padding, and Spacer
usage accordingly so only one mechanism controls the vertical space.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Home/HomeViewModel.swift`:
- Around line 73-76: challengeBarImageName uses cherryLevel directly which can
be out of bounds and produce missing image names; update challengeBarImageName
to clamp cherryLevel into the valid 0...4 range (same logic used in
cherryLevelImageName) before building the string, e.g., compute a safeLevel =
max(0, min(cherryLevel, 4)) and return "home_chellenge_bar\(safeLevel)"; locate
the challengeBarImageName computed property and apply this clamping so it never
references non-existent images.
In `@Cherrish-iOS/Cherrish-iOS/Presentation/Global/Enum/CherryLevel.swift`:
- Around line 27-42: The from(progressRate:) switch has overlapping cases and
doesn't handle out-of-range inputs clearly; update
CherryLevel.from(progressRate:) to first clamp progressRate to 0...100 (or
explicitly handle negatives and >100), then use non-overlapping boundaries such
as if progressRate <= 0 { return .level0 } else if progressRate < 25 { return
.level1 } else if progressRate < 50 { return .level2 } else if progressRate < 75
{ return .level3 } else /* <=100 */ { return .level4 }, removing the redundant
case 0 and the catch-all default so ranges are explicit and clear.
In
`@Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/ChallengeImage/challenge_gaugebar_0.imageset/Contents.json`:
- Around line 1-21: The progressImage lookup in CherryLevel.swift
(Image("challenge_gaugebar_\(rawValue)") used around the progressImage property)
fails for level 0 because the asset file is named home_chellenge_bar0.svg inside
the challenge_gaugebar_0 asset folder; either rename that SVG to match the
expected asset name "challenge_gaugebar_0" (and add 2x/3x variants or
appropriate PDFs) or change the reference in CherryLevel.swift for rawValue 0 to
the actual asset name; ensure the asset set contains 1x/2x/3x entries so
Image(...) can load at all scales.
Comment on lines
73
to
76
| var challengeBarImageName: String { | ||
| let level = min(max(cherryLevel, 0), 4) | ||
| let level = cherryLevel | ||
| return "home_chellenge_bar\(level)" | ||
| } |
There was a problem hiding this comment.
challengeBarImageName에서 cherryLevel 경계값 검증 누락
이전에는 cherryLevel을 클램핑(clamping)하여 사용했을 가능성이 있는데, 현재는 직접 사용하고 있습니다. 서버에서 반환되는 cherryLevel이 0~4 범위를 벗어나는 경우, 존재하지 않는 이미지(home_chellenge_bar5 등)를 참조할 수 있습니다.
cherryLevelImageName(Line 79)에서는 0인 경우를 처리하고 있으나, challengeBarImageName에서는 동일한 보호가 없습니다.
♻️ 경계값 검증 추가 제안
var challengeBarImageName: String {
- let level = cherryLevel
+ let level = min(max(cherryLevel, 0), 4)
return "home_chellenge_bar\(level)"
}🤖 Prompt for AI Agents
In `@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Home/HomeViewModel.swift`
around lines 73 - 76, challengeBarImageName uses cherryLevel directly which can
be out of bounds and produce missing image names; update challengeBarImageName
to clamp cherryLevel into the valid 0...4 range (same logic used in
cherryLevelImageName) before building the string, e.g., compute a safeLevel =
max(0, min(cherryLevel, 4)) and return "home_chellenge_bar\(safeLevel)"; locate
the challengeBarImageName computed property and apply this clamping so it never
references non-existent images.
Comment on lines
+27
to
+42
| static func from(progressRate: Double) -> CherryLevel { | ||
| switch progressRate { | ||
| case 0: | ||
| return .level0 | ||
| case 0.0..<25.0: | ||
| return .level1 | ||
| case 25.0..<50.0: | ||
| return .level2 | ||
| case 50.0..<75.0: | ||
| return .level3 | ||
| case 75.0...100.0: | ||
| return .level4 | ||
| default: | ||
| return .level1 | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
from(progressRate:) 메서드의 케이스 중복 및 경계값 처리 개선 필요
case 0과 case 0.0..<25.0이 모두 progressRate == 0을 포함합니다. Swift의 switch는 첫 번째 매칭 케이스에서 종료되므로 현재 동작은 의도대로 작동하지만, 코드 가독성과 의도 명확성을 위해 범위를 명시적으로 분리하는 것이 좋습니다.
또한 progressRate가 음수이거나 100을 초과하는 경우 default로 .level1을 반환하는데, 이것이 의도된 동작인지 확인이 필요합니다.
♻️ 범위를 명확히 분리한 개선안
static func from(progressRate: Double) -> CherryLevel {
switch progressRate {
case 0:
return .level0
- case 0.0..<25.0:
+ case 0.0<..<25.0: // 0 초과 ~ 25 미만 (Swift에서는 이 문법이 지원되지 않음)
return .level1Swift에서는 0.0<..<25.0 문법이 지원되지 않으므로, 다음과 같이 수정하는 것을 권장합니다:
static func from(progressRate: Double) -> CherryLevel {
- switch progressRate {
- case 0:
- return .level0
- case 0.0..<25.0:
- return .level1
- case 25.0..<50.0:
- return .level2
- case 50.0..<75.0:
- return .level3
- case 75.0...100.0:
- return .level4
- default:
- return .level1
- }
+ if progressRate == 0 {
+ return .level0
+ } else if progressRate < 25.0 {
+ return .level1
+ } else if progressRate < 50.0 {
+ return .level2
+ } else if progressRate < 75.0 {
+ return .level3
+ } else if progressRate <= 100.0 {
+ return .level4
+ } else {
+ return .level1
+ }
}📝 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
| static func from(progressRate: Double) -> CherryLevel { | |
| switch progressRate { | |
| case 0: | |
| return .level0 | |
| case 0.0..<25.0: | |
| return .level1 | |
| case 25.0..<50.0: | |
| return .level2 | |
| case 50.0..<75.0: | |
| return .level3 | |
| case 75.0...100.0: | |
| return .level4 | |
| default: | |
| return .level1 | |
| } | |
| } | |
| static func from(progressRate: Double) -> CherryLevel { | |
| if progressRate == 0 { | |
| return .level0 | |
| } else if progressRate < 25.0 { | |
| return .level1 | |
| } else if progressRate < 50.0 { | |
| return .level2 | |
| } else if progressRate < 75.0 { | |
| return .level3 | |
| } else if progressRate <= 100.0 { | |
| return .level4 | |
| } else { | |
| return .level1 | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@Cherrish-iOS/Cherrish-iOS/Presentation/Global/Enum/CherryLevel.swift` around
lines 27 - 42, The from(progressRate:) switch has overlapping cases and doesn't
handle out-of-range inputs clearly; update CherryLevel.from(progressRate:) to
first clamp progressRate to 0...100 (or explicitly handle negatives and >100),
then use non-overlapping boundaries such as if progressRate <= 0 { return
.level0 } else if progressRate < 25 { return .level1 } else if progressRate < 50
{ return .level2 } else if progressRate < 75 { return .level3 } else /* <=100 */
{ return .level4 }, removing the redundant case 0 and the catch-all default so
ranges are explicit and clear.
Comment on lines
+1
to
+21
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "home_chellenge_bar0.svg", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the asset file and related image files
find . -type f -name "*chellenge*" -o -name "*gaugebar*" 2>/dev/null | head -20Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 2432
🏁 Script executed:
# List the contents of the challenge asset folder
ls -la Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/ChallengeImage/challenge_gaugebar_0.imageset/ 2>/dev/null || echo "Path not found, searching for similar paths"
fd challenge_gaugebar Cherrish-iOS 2>/dev/nullRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 2299
🏁 Script executed:
# Search for references to these images in Swift code
rg -n "home_chellenge_bar|challenge_gaugebar" --type swift 2>/dev/null | head -30Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 308
🏁 Script executed:
# Check for all imageset Contents.json files to understand the naming pattern
fd -e json "Contents\.json" Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets 2>/dev/null | xargs rg -l "home_chellenge|challenge_gaugebar" 2>/dev/nullRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 1149
🏁 Script executed:
cat -n Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/ChallengeImage/challenge_gaugebar_0.imageset/Contents.jsonRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 533
🏁 Script executed:
# Check CherryLevel.swift to see the rawValue mapping
sed -n '55,75p' Cherrish-iOS/Cherrish-iOS/Presentation/Global/Enum/CherryLevel.swiftRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 371
🏁 Script executed:
# Check HomeViewModel.swift context around the image reference
sed -n '70,85p' Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Home/HomeViewModel.swiftRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 591
🏁 Script executed:
# Check one of the properly formed challenge_gaugebar Contents.json files for comparison
cat Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/ChallengeImage/challenge_gaugebar_1.imageset/Contents.jsonRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 487
CherryLevel.swift의 progressImage가 존재하지 않는 에셋을 참조합니다
CherryLevel.swift 라인 64에서 Image("challenge_gaugebar_\(rawValue)")로 참조하려는 에셋 이름이 Contents.json의 실제 파일명과 불일치합니다.
- 예상되는 참조:
challenge_gaugebar_0 - 실제 에셋 이름:
home_chellenge_bar0.svg(challenge_gaugebar_0 폴더 내)
이로 인해 level 0일 때 progressImage 로드가 실패할 수 있습니다. challenge_gaugebar_1~4는 올바른 이름 패턴을 따르므로, challenge_gaugebar_0도 동일하게 수정되거나 CherryLevel.swift에서 올바른 에셋 이름을 참조해야 합니다.
추가로 challenge_gaugebar_0는 1x 스케일만 파일을 가지고 있으므로 2x, 3x 파일도 필요합니다.
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Resources/Assets.xcassets/ChallengeImage/challenge_gaugebar_0.imageset/Contents.json`
around lines 1 - 21, The progressImage lookup in CherryLevel.swift
(Image("challenge_gaugebar_\(rawValue)") used around the progressImage property)
fails for level 0 because the asset file is named home_chellenge_bar0.svg inside
the challenge_gaugebar_0 asset folder; either rename that SVG to match the
expected asset name "challenge_gaugebar_0" (and add 2x/3x variants or
appropriate PDFs) or change the reference in CherryLevel.swift for rawValue 0 to
the actual asset name; ensure the asset set contains 1x/2x/3x entries so
Image(...) can load at all scales.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 연결된 이슈
📄 작업 내용