Modernize Swift codelab - #248
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modernizes the FriendlyChat iOS codelab by migrating the Swift starter and completed projects from UIKit, Storyboards, and CocoaPods to SwiftUI, Swift Package Manager, and Firebase iOS SDK v12+. Key changes include the removal of legacy view controllers and storyboards, the introduction of SwiftUI views and modern MVVM view models, and updates to the test script. The review feedback highlights several areas for improvement, including handling Google Storage URIs in image loading, preventing out-of-bounds scrolling when messages are empty, replacing deprecated text modifiers, ensuring thread safety for main-actor-isolated state mutations in database observers, and using bash arrays in the test script to avoid word-splitting bugs.
| } else { | ||
| let storageRef = Storage.storage().reference(withPath: imageUrl) | ||
| if let data = try? await storageRef.data(maxSize: 5 * 1024 * 1024), | ||
| let downloadedImage = UIImage(data: data) { | ||
| self.image = downloadedImage | ||
| } | ||
| } |
There was a problem hiding this comment.
Using Storage.storage().reference(withPath:) directly with imageUrl will fail if the URL is a full Google Storage URI (starting with gs://). To ensure robustness and support both relative paths and full URIs, check the prefix and use reference(forURL:) when appropriate.
} else {
let storageRef: StorageReference
if imageUrl.hasPrefix("gs://") {
storageRef = Storage.storage().reference(forURL: imageUrl)
} else {
storageRef = Storage.storage().reference(withPath: imageUrl)
}
if let data = try? await storageRef.data(maxSize: 5 * 1024 * 1024),
let downloadedImage = UIImage(data: data) {
self.image = downloadedImage
}
}| .onChange(of: messageViewModel.messages.count) { _ in | ||
| withAnimation(.easeInOut) { | ||
| scrollViewReader.scrollTo(messageViewModel.messages.count - 1, anchor: .bottom) | ||
| } | ||
| } |
There was a problem hiding this comment.
When messageViewModel.messages is empty, messages.count - 1 evaluates to -1. Attempting to scroll to an invalid index of -1 can cause unexpected behavior or console warnings. Adding a guard check prevents this issue.
.onChange(of: messageViewModel.messages.count) { count in
guard count > 0 else { return }
withAnimation(.easeInOut) {
scrollViewReader.scrollTo(count - 1, anchor: .bottom)
}
}| .autocapitalization(.none) | ||
| .disableAutocorrection(true) |
There was a problem hiding this comment.
The .autocapitalization(_:) and .disableAutocorrection(_:) modifiers are deprecated in iOS 16.0. Since the deployment target is set to iOS 16.0, use the modern .textInputAutocapitalization(_:) and .autocorrectionDisabled(_:) modifiers instead.
| .autocapitalization(.none) | |
| .disableAutocorrection(true) | |
| .textInputAutocapitalization(.never) | |
| .autocorrectionDisabled(true) |
| .autocapitalization(.none) | ||
| .disableAutocorrection(true) |
There was a problem hiding this comment.
The .autocapitalization(_:) and .disableAutocorrection(_:) modifiers are deprecated in iOS 16.0. Since the deployment target is set to iOS 16.0, use the modern .textInputAutocapitalization(_:) and .autocorrectionDisabled(_:) modifiers instead.
| .autocapitalization(.none) | |
| .disableAutocorrection(true) | |
| .textInputAutocapitalization(.never) | |
| .autocorrectionDisabled(true) |
| refHandle = dbRef.observe(.childAdded) { [weak self] snapshot in | ||
| guard let self = self, | ||
| var dict = snapshot.value as? [String: Any] else { return } | ||
| dict["id"] = snapshot.key | ||
| if let data = try? JSONSerialization.data(withJSONObject: dict), | ||
| let message = try? JSONDecoder().decode(FriendlyMessage.self, from: data) { | ||
| self.messages.append(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since FriendlyMessageViewModel is isolated to @MainActor, mutating self.messages directly inside the escaping closure of dbRef.observe can lead to data races and compiler errors under strict concurrency checking (Swift 6). Wrapping the state mutation in a Task { @MainActor in ... } block ensures thread safety.
refHandle = dbRef.observe(.childAdded) { [weak self] snapshot in
guard let self = self,
var dict = snapshot.value as? [String: Any] else { return }
dict["id"] = snapshot.key
if let data = try? JSONSerialization.data(withJSONObject: dict),
let message = try? JSONDecoder().decode(FriendlyMessage.self, from: data) {
Task { @MainActor in
self.messages.append(message)
}
}
}| if [ -d "${DIR}/FriendlyChat${LANGUAGE}.xcworkspace" ]; then | ||
| BUILD_TARGET="-workspace ${DIR}/FriendlyChat${LANGUAGE}.xcworkspace" | ||
| else | ||
| BUILD_TARGET="-project ${DIR}/FriendlyChat${LANGUAGE}.xcodeproj" | ||
| fi |
There was a problem hiding this comment.
Using unquoted string variables for command arguments like BUILD_TARGET can lead to word-splitting bugs if paths contain spaces. Using a bash array is the standard and robust way to handle multi-part command arguments safely.
| if [ -d "${DIR}/FriendlyChat${LANGUAGE}.xcworkspace" ]; then | |
| BUILD_TARGET="-workspace ${DIR}/FriendlyChat${LANGUAGE}.xcworkspace" | |
| else | |
| BUILD_TARGET="-project ${DIR}/FriendlyChat${LANGUAGE}.xcodeproj" | |
| fi | |
| if [ -d "${DIR}/FriendlyChat${LANGUAGE}.xcworkspace" ]; then | |
| BUILD_TARGET=(-workspace "${DIR}/FriendlyChat${LANGUAGE}.xcworkspace") | |
| else | |
| BUILD_TARGET=(-project "${DIR}/FriendlyChat${LANGUAGE}.xcodeproj") | |
| fi |
|
|
||
| (xcodebuild \ | ||
| -workspace ${DIR}/FriendlyChat${LANGUAGE}.xcworkspace \ | ||
| ${BUILD_TARGET} \ |
There was a problem hiding this comment.
When using a bash array for BUILD_TARGET, expand it correctly as "${BUILD_TARGET[@]}" to preserve arguments with spaces.
| ${BUILD_TARGET} \ | |
| "${BUILD_TARGET[@]}" \ |
|
Apart from my comment about Observation, I'd recommend installing the following skills for up-to-date SwiftUI / Swift Concurrency / view design insights:
and/or using the Xcode 27 skills |
2 participants
DO_NOT_MERGE: codelab text coming soon