feat: ios app #37
feat: ios app #37
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@krystxf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces several changes across multiple files in the metro-now application. A new SwiftUI view, Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (27)
apps/mobile/metro-now/common/utils/metro-line.utils.swift (2)
Line range hint
6-12: Consider making the nil case explicit in the switch statement.The default case implicitly handles both the
.nonecase and any future metro lines. For better maintainability and clarity, consider making these cases explicit.func getMetroLineColor(_ line: MetroLine?) -> Color? { switch line { case .A: .green case .B: .yellow case .C: .red - default: nil + case .none: nil + // Add new lines here } }
Line range hint
1-17: Consider adding unit tests for the color mapping utility.This utility is crucial for the visual representation of metro lines in the app. Adding unit tests would help ensure reliability, especially when handling edge cases and future metro line additions.
Would you like me to help create a comprehensive test suite for this utility?
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-departures-list.view.swift (2)
1-3: Add comprehensive file documentation.Consider adding a documentation block that includes:
- View's purpose and responsibilities
- Usage example
- Parameter documentation for
departures// metro-now // https://github.com/krystxf/metro-now + +/// A view that displays a list of metro departures for a Watch App interface. +/// +/// Example usage: +/// ```swift +/// PlatformDetailDeparturesListView(departures: departures) +/// ``` +/// +/// - Parameter departures: An array of ApiDeparture objects containing departure information
6-16: Consider Watch-specific optimizations.For optimal Watch App performance and user experience:
- Consider limiting the number of visible departures to reduce memory usage and improve performance
- Add pull-to-refresh for manual updates instead of continuous polling
- Consider using larger text for better readability on the small Watch screen
struct PlatformDetailDeparturesListView: View { let departures: [ApiDeparture] + + // Limit visible departures for better performance + private var visibleDepartures: [ApiDeparture] { + Array(departures.prefix(5)) + } var body: some View { - List(departures, id: \.departure.predicted) { departure in + List(visibleDepartures, id: \.id) { departure in HStack { Text(departure.headsign) + .font(.system(size: 16, weight: .medium)) Spacer() CountdownView(targetDate: departure.departure.predicted) } } + .refreshable { + // TODO: Implement refresh logic + } } }apps/mobile/metro-now/common/components/route-name.view.swift (2)
6-9: Add documentation comments for better reusability.Consider adding documentation comments to describe the purpose of the view and its parameters, which would help other developers understand how to use this component.
+/// A view that displays a system icon with a colored background and white border +/// Used for representing metro route identifiers struct RouteNameIconView: View { + /// The SF Symbol name to be displayed let systemName: String + /// The background color of the icon container let background: Color
24-29: Enhance preview with real-world examples.The current preview uses a simple example. Consider expanding it to show how the component looks with different route identifiers and colors that match actual metro lines.
#Preview { - RouteNameIconView( - systemName: "a", - background: .green - ) + VStack(spacing: 10) { + RouteNameIconView(systemName: "A", background: .green) + RouteNameIconView(systemName: "B", background: .yellow) + RouteNameIconView(systemName: "C", background: .red) + RouteNameIconView(systemName: "1", background: .blue) + } + .padding() + .background(Color(.systemBackground)) }apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item-placeholder.view.swift (4)
1-4: Consider adding a brief documentation comment.Adding a brief description of the view's purpose and usage would improve code maintainability.
// metro-now // https://github.com/krystxf/metro-now + +/// A placeholder view that displays a loading state for stop departures +/// Used while the actual departure data is being fetched
9-12: Fix initializer parameter formatting.The parameter declaration should be on the same line as
init.- init(color: Color? - ) { + init(color: Color?) {
14-23: Consider using a static date for placeholders.Using
.nowfor placeholder dates might cause unnecessary view updates. Consider using a static date since this is just a placeholder.StopDepartureListItemView( color: color, headsign: "Loading", - departure: .now, + departure: Date(timeIntervalSince1970: 0), nextHeadsign: "Loading", - nextDeparture: .now + nextDeparture: Date(timeIntervalSince1970: 0) )
26-41: Enhance preview with dark mode variant.Consider adding a dark mode preview to ensure the placeholder looks good in both appearance modes.
#Preview { - ScrollView { - StopDepartureListItemPlaceholderView( - color: .red - ) - StopDepartureListItemPlaceholderView( - color: .red - ) - StopDepartureListItemPlaceholderView( - color: .green - ) - StopDepartureListItemPlaceholderView( - color: .green - ) + Group { + ScrollView { + StopDepartureListItemPlaceholderView(color: .red) + StopDepartureListItemPlaceholderView(color: .red) + StopDepartureListItemPlaceholderView(color: .green) + StopDepartureListItemPlaceholderView(color: .green) + } + .previewDisplayName("Light Mode") + + ScrollView { + StopDepartureListItemPlaceholderView(color: .red) + StopDepartureListItemPlaceholderView(color: .red) + StopDepartureListItemPlaceholderView(color: .green) + StopDepartureListItemPlaceholderView(color: .green) + } + .preferredColorScheme(.dark) + .previewDisplayName("Dark Mode") } }apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift (1)
Line range hint
27-53: Consider adding accessibility improvements.While the layout is clean and well-structured, consider enhancing accessibility for VoiceOver users.
Add accessibility labels to improve the experience:
HStack { Text(shortenStopName(headsign)) + .accessibilityLabel("Destination: \(headsign)") Spacer() CountdownView(targetDate: departure) + .accessibilityLabel("Departing in \(formatAccessibilityTime(departure))") }apps/mobile/metro-now/metro-now Watch App/pages/platform/next-departure.view.swift (2)
1-5: Add documentation to improve code maintainability.Consider adding:
- Copyright notice
- Brief description of the view's purpose and usage
- Documentation comments that explain the view's role in the watch app
// metro-now // https://github.com/krystxf/metro-now +/// A view that displays the next departure information for a transit platform. +/// This view is designed specifically for the Watch App interface, showing both +/// current and next departure times with their respective destinations. + import SwiftUI
6-11: Add property documentation and consider date validation.The properties would benefit from documentation comments. Also, consider adding validation to ensure
nextDepartureis afterdeparturewhen both are present.struct PlatformDetailNextDepartureView: View { + /// The destination or direction of the current departure let headsign: String + /// The scheduled time of the current departure let departure: Date + /// The destination or direction of the next departure, if available let nextHeadsign: String? + /// The scheduled time of the next departure, if available let nextDeparture: Date? + + init(headsign: String, departure: Date, nextHeadsign: String? = nil, nextDeparture: Date? = nil) { + self.headsign = headsign + self.departure = departure + self.nextHeadsign = nextHeadsign + self.nextDeparture = nextDeparture + + if let nextDeparture, nextDeparture <= departure { + assertionFailure("nextDeparture must be after departure") + } + }apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (3)
6-15: Consider adding documentation for data models.Adding documentation comments explaining the purpose of these structs and their properties would improve code maintainability.
+/// Represents a single metro departure struct Departure { let id: String let headsign: String } +/// Represents a metro platform with its associated line and departures struct MainPagePlatform { let id: String let metroLine: MetroLine? let departures: [ApiDeparture]? }
21-21: Remove redundant nil initialization.The optional State variable doesn't need explicit nil initialization.
- @State var selectedPlatformId: String? = nil + @State var selectedPlatformId: String?🧰 Tools
🪛 SwiftLint
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
68-94: Consider enhancing preview with actual departure data.The current preview shows only platforms with nil departures. Adding some sample departure data would help visualize the actual user experience.
MainPagePlatform( id: "U689Z101P", metroLine: .B, - departures: nil + departures: [ + ApiDeparture( + headsign: "Zličín", + departure: Departure(predicted: Date().addingTimeInterval(120)) + ), + ApiDeparture( + headsign: "Černý Most", + departure: Departure(predicted: Date().addingTimeInterval(300)) + ) + ] ),apps/mobile/metro-now/metro-now Watch App/ContentView.swift (3)
Line range hint
20-31: Consider extracting platform mapping logic to improve readability and performance.The nested closures and inline filtering make this section harder to maintain. Consider extracting the platform mapping logic to a separate method.
Here's a suggested refactor:
- StopDeparturesView( - title: closestStop.name, - platforms: closestStop.platforms.map { - platform in - MainPagePlatform( - id: platform.id, - metroLine: MetroLine(rawValue: platform.routes[0].name), - departures: departures?.filter { departure in - departure.platformId == platform.id - } - ) - } - ) + StopDeparturesView( + title: closestStop.name, + platforms: mapPlatformsToMainPage( + platforms: closestStop.platforms, + departures: departures + ) + ) + private func mapPlatformsToMainPage( + platforms: [Platform], + departures: [ApiDeparture]? + ) -> [MainPagePlatform] { + platforms.map { platform in + MainPagePlatform( + id: platform.id, + metroLine: MetroLine(rawValue: platform.routes[0].name), + departures: departures?.filter { $0.platformId == platform.id } + ) + } + }
Line range hint
33-37: Consider optimizing update frequency for battery life.The 2-second refresh interval for departure data might be too aggressive for a watch app, potentially impacting battery life and generating unnecessary API calls.
Consider:
- Increasing the refresh interval to 30-60 seconds
- Adding error backoff logic for failed API calls
- Implementing rate limiting to prevent excessive API usage
Line range hint
82-92: Add user-facing error handling.Currently, network errors are only logged to the console. Watch OS users need clear feedback when something goes wrong.
Consider implementing error states in the UI:
struct ContentView: View { + @State private var error: Error? + @State private var showError = false // ... existing properties ... func getAllMetroStops() { NetworkManager.shared.getMetroStops { result in DispatchQueue.main.async { switch result { case let .success(stops): self.stops = stops case let .failure(error): - print(error.localizedDescription) + self.error = error + self.showError = true } } } } }Then add an error alert to your view:
.alert("Error", isPresented: $showError, presenting: error) { _ in Button("OK") { showError = false } } message: { error in Text(error.localizedDescription) }Also applies to: 104-114
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift (4)
1-5: Add comprehensive file documentation.Consider adding:
- Copyright/license information
- Brief description of the view's purpose and responsibilities
- Any important usage notes or requirements
// metro-now // https://github.com/krystxf/metro-now +// +// Copyright © 2024 metro-now. All rights reserved. +// +/// PlatformDetailView displays real-time departure information for a specific metro platform. +/// It shows the next departure and a list of upcoming departures in a tabbed interface. import SwiftUI
6-11: Document properties and remove redundant initialization.The properties should be documented for better maintainability, and the redundant nil initialization can be removed.
struct PlatformDetailView: View { + /// Unique identifier for the metro platform let platformId: String + /// Metro line associated with this platform (optional) let metroLine: MetroLine? - @State var departures: [ApiDeparture]? = nil + /// Current departures for this platform + @State var departures: [ApiDeparture]? + /// Timer for refreshing departure data every 2 seconds private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect()🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
90-97: Enhance preview with multiple scenarios.Consider adding previews for different states:
- Loading state
- Error state
- Empty departures
- Multiple departures
#Preview { - NavigationStack { - PlatformDetailView( - platformId: "U1040Z101P", - metroLine: MetroLine.B - ) + Group { + NavigationStack { + PlatformDetailView( + platformId: "U1040Z101P", + metroLine: MetroLine.B, + departures: [ + ApiDeparture(/* sample data */), + ApiDeparture(/* sample data */) + ] + ) + } + .previewDisplayName("With Departures") + + NavigationStack { + PlatformDetailView( + platformId: "U1040Z101P", + metroLine: MetroLine.B + ) + } + .previewDisplayName("Loading State") } }
6-88: Consider architectural improvements.
The 2-second refresh interval might be too aggressive for a watch app, potentially impacting battery life. Consider:
- Increasing the interval (e.g., 5-10 seconds)
- Using push notifications for updates
- Adding a manual refresh option
Move business logic to a dedicated ViewModel:
- Separate data fetching and transformation
- Improve testability
- Better state management
Would you like me to help create a ViewModel implementation for this view?
🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now/ContentView.swift (4)
10-11: Remove redundant initialization of optional variablesInitializing optional variables with
nilis redundant since they arenilby default. Removing the explicit initialization enhances code clarity.Apply this diff to remove the redundant initializations:
- @State var stops: [ApiStop]? = nil - @State var departures: [ApiDeparture]? = nil + @State var stops: [ApiStop]? + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 11-11: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
9-9: Reconsider the polling interval to reduce resource usagePolling for departures every 2 seconds may lead to increased battery consumption and network usage. Consider increasing the interval or implementing a more efficient update mechanism, such as pushing updates only when data changes or using reactive programming with
Combine.Also applies to: 88-90
124-126: Improve user feedback by handling network errors appropriatelyCurrently, network errors are only printed to the console. Providing user feedback, such as displaying an alert or message, can enhance the user experience by informing them of any issues.
You might consider implementing an error state in the UI or using an
@Statevariable to trigger alerts.Also applies to: 148-150
33-59: Simplify nested conditionals in UI code for better readabilityThe nested conditional statements in the UI code increase complexity and reduce readability. Consider refactoring to improve maintainability, possibly by extracting repeated patterns into custom views or simplifying the logic.
For example, you could create a custom view for the departure rows to reduce duplication and make the code more modular.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
apps/mobile/metro-now/common/components/route-name.view.swift(1 hunks)apps/mobile/metro-now/common/utils/metro-line.utils.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/ContentView.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/departure-placeholder.swift(0 hunks)apps/mobile/metro-now/metro-now Watch App/pages/main-page.swift(0 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/next-departure.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-departures-list.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item-placeholder.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift(2 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj(11 hunks)apps/mobile/metro-now/metro-now/ContentView.swift(1 hunks)
💤 Files with no reviewable changes (2)
- apps/mobile/metro-now/metro-now Watch App/pages/departure-placeholder.swift
- apps/mobile/metro-now/metro-now Watch App/pages/main-page.swift
🧰 Additional context used
🪛 SwiftLint
apps/mobile/metro-now/common/components/route-name.view.swift
[Warning] 11-11: TODOs should be resolved (replace with Text component fo...)
(todo)
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now/ContentView.swift
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 11-11: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (12)
apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item-placeholder.view.swift (1)
6-24: Implementation looks good!
The placeholder view is well-implemented, following SwiftUI best practices. It provides a clean loading state for the stop departures list with good customization options through the color parameter.
apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift (2)
Line range hint 6-25: LGTM! Well-structured view declaration with improved naming.
The renaming to StopDepartureListItemView better follows SwiftUI naming conventions, and the optional color parameter with a default value improves API ergonomics.
54-62: Verify list spacing with zero insets.
Setting all list row insets to zero might affect the visual separation between items. Please ensure this doesn't compromise readability on the small watch screen.
Consider adding a small horizontal inset (e.g., 4-8 points) to maintain visual hierarchy and prevent content from touching screen edges, which is a common practice in watchOS design.
apps/mobile/metro-now/metro-now Watch App/pages/platform/next-departure.view.swift (1)
12-52: Verify external dependencies and enhance accessibility.
The implementation looks clean, but there are a few considerations:
Consider these improvements:
- Add accessibility labels for VoiceOver support
- Consider adding a preview provider for SwiftUI previews
- Consider extracting string literals to localization
Here's how to implement these improvements:
var body: some View {
VStack(spacing: 40) {
VStack {
Spacer()
Label(
shortenStopName(headsign),
systemImage: "arrowshape.forward"
- ).font(.title2)
+ )
+ .font(.title2)
+ .accessibilityLabel("Next departure to \(headsign)")
// ... rest of the implementation
}
}
}
+#Preview {
+ PlatformDetailNextDepartureView(
+ headsign: "Example Station",
+ departure: Date(),
+ nextHeadsign: "Next Station",
+ nextDeparture: Date().addingTimeInterval(900)
+ )
+}✅ Verification successful
Function exists but needs improvement for maintainability
The shortenStopName function exists in apps/mobile/metro-now/common/utils/station.utils.swift but uses hardcoded string comparisons. Let's verify if there are more cases to understand the full scope.
Function exists and is used consistently across watch app views
The shortenStopName function is properly implemented in station.utils.swift and is consistently used across multiple watch app views for displaying shortened station names. The implementation handles both specific cases and general patterns (like "Nádraží"/"nádraží" prefixes/suffixes) appropriately.
The original review's accessibility suggestions remain valid, but the external dependency concern is resolved:
- The function exists and is properly shared
- It's used consistently across the watch app
- The implementation is reasonable for the use case
Consider implementing the suggested accessibility improvements from the original review, but the external dependency verification is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and implementation of shortenStopName function
rg -A 5 "func shortenStopName"
Length of output: 592
Script:
#!/bin/bash
# Get the complete implementation of shortenStopName
rg -A 15 "func shortenStopName"
# Check for other usages of this function
rg "shortenStopName\("
Length of output: 2121
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (3)
1-5: LGTM! Clean imports and good documentation.
The file header with repository link provides good context, and imports are minimal and appropriate.
23-65: LGTM! Well-structured view implementation.
The view implementation follows SwiftUI best practices:
- Uses NavigationSplitView appropriately for master-detail layout
- Handles empty states with placeholder views
- Properly manages state with @State property wrapper
26-26: Verify external function dependencies.
The code relies on external utility functions getMetroLineColor and shortenStopName. Let's verify their implementations.
Also applies to: 46-46
✅ Verification successful
Let me check the implementation of these utility functions to ensure they are properly implemented.
External utility functions are properly implemented and safe to use
Both getMetroLineColor and shortenStopName functions are well-implemented in their respective utility files:
getMetroLineColoris defined inmetro-line.utils.swiftwith proper type safety and color mappingsshortenStopNameis defined instation.utils.swiftwith clear string transformations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the utility functions implementation
echo "Searching for getMetroLineColor function..."
rg -t swift "func getMetroLineColor"
echo "Searching for shortenStopName function..."
rg -t swift "func shortenStopName"
Length of output: 591
Script:
#!/bin/bash
# Check the implementation of getMetroLineColor
echo "Implementation of getMetroLineColor:"
rg -A 10 "func getMetroLineColor" apps/mobile/metro-now/common/utils/metro-line.utils.swift
echo -e "\nImplementation of shortenStopName:"
rg -A 10 "func shortenStopName" apps/mobile/metro-now/common/utils/station.utils.swift
Length of output: 958
apps/mobile/metro-now/metro-now Watch App/ContentView.swift (1)
16-18: LGTM! Clean and idiomatic optional handling.
The conditional checks are well-structured and follow Swift best practices for optional binding.
apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj (4)
445-446: LGTM: Interface orientation settings
The orientation settings are well-configured:
- iPhone: Portrait-only is appropriate for a metro navigation app
- iPad: Full orientation support provides flexibility for tablet users
Also applies to: 479-480
283-283: LGTM: Dead code stripping enabled
Enabling DEAD_CODE_STRIPPING is good for optimizing the binary size by removing unused code.
Also applies to: 345-345
47-47: LGTM: Route name view exception added
The addition of "components/route-name.view.swift" to the Watch App's exceptions list is correctly configured.
392-392: Verify watchOS 11 requirement
Increasing the minimum watchOS requirement from 9 to 11 is a significant jump that could exclude many users. Please verify if watchOS 11-specific features are actually required.
Also applies to: 424-424
| func getMetroLineColor(_ line: String) -> Color? { | ||
| getMetroLineColor(MetroLine(rawValue: line.uppercased())) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation for the string overload.
The function could benefit from input validation and documentation explaining the expected string format. Currently, it silently returns nil for invalid inputs.
+/// Converts a string representation of a metro line (e.g., "A", "B", "C") to its corresponding color.
+/// - Parameter line: The metro line identifier as a string (case-insensitive)
+/// - Returns: The color associated with the metro line, or nil if the line is invalid
func getMetroLineColor(_ line: String) -> Color? {
+ guard !line.isEmpty else { return nil }
getMetroLineColor(MetroLine(rawValue: line.uppercased()))
}📝 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.
| func getMetroLineColor(_ line: String) -> Color? { | |
| getMetroLineColor(MetroLine(rawValue: line.uppercased())) | |
| } | |
| /// Converts a string representation of a metro line (e.g., "A", "B", "C") to its corresponding color. | |
| /// - Parameter line: The metro line identifier as a string (case-insensitive) | |
| /// - Returns: The color associated with the metro line, or nil if the line is invalid | |
| func getMetroLineColor(_ line: String) -> Color? { | |
| guard !line.isEmpty else { return nil } | |
| getMetroLineColor(MetroLine(rawValue: line.uppercased())) | |
| } |
| var body: some View { List(departures, id: \.departure.predicted) { departure in | ||
| HStack { | ||
| Text(departure.headsign) | ||
| Spacer() | ||
| CountdownView(targetDate: departure.departure.predicted) | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve list implementation for better reliability and accessibility.
Several improvements are needed for better Watch App user experience:
- Using predicted time as ID could cause issues if multiple departures share the same time
- Missing empty state handling
- Needs accessibility support
- var body: some View { List(departures, id: \.departure.predicted) { departure in
+ var body: some View {
+ List {
+ if departures.isEmpty {
+ Text("No departures available")
+ .foregroundColor(.secondary)
+ } else {
+ ForEach(departures, id: \.id) { departure in
+ HStack {
+ Text(departure.headsign)
+ .accessibilityLabel("Destination: \(departure.headsign)")
+ Spacer()
+ CountdownView(targetDate: departure.departure.predicted)
+ .accessibilityLabel("Departing in: ")
+ }
+ }
+ }
+ }
+ .listStyle(.plain)
+ .scrollIndicators(.visible)Committable suggestion skipped: line range outside the PR's diff.
| var body: some View { | ||
| Image(systemName: systemName) // TODO: replace with Text component for more flexibility | ||
| .imageScale(.medium) | ||
| .padding(5) | ||
| .foregroundStyle(.white) | ||
| .background(background) | ||
| .clipShape(.rect(cornerRadius: 6)) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: 6) | ||
| .stroke(.white, lineWidth: 2) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Image with Text for better flexibility.
The current implementation using Image(systemName:) limits the display to SF Symbols. Using Text would allow for displaying any route identifier format (numbers, letters, or combinations).
Here's a suggested implementation:
var body: some View {
- Image(systemName: systemName)
- .imageScale(.medium)
+ Text(systemName)
+ .font(.system(size: 16, weight: .medium))
.padding(5)
.foregroundStyle(.white)
.background(background)
.clipShape(.rect(cornerRadius: 6))
.overlay(
RoundedRectangle(cornerRadius: 6)
.stroke(.white, lineWidth: 2)
)
}📝 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.
| var body: some View { | |
| Image(systemName: systemName) // TODO: replace with Text component for more flexibility | |
| .imageScale(.medium) | |
| .padding(5) | |
| .foregroundStyle(.white) | |
| .background(background) | |
| .clipShape(.rect(cornerRadius: 6)) | |
| .overlay( | |
| RoundedRectangle(cornerRadius: 6) | |
| .stroke(.white, lineWidth: 2) | |
| ) | |
| } | |
| } | |
| var body: some View { | |
| Text(systemName) | |
| .font(.system(size: 16, weight: .medium)) | |
| .padding(5) | |
| .foregroundStyle(.white) | |
| .background(background) | |
| .clipShape(.rect(cornerRadius: 6)) | |
| .overlay( | |
| RoundedRectangle(cornerRadius: 6) | |
| .stroke(.white, lineWidth: 2) | |
| ) | |
| } | |
| } |
🧰 Tools
🪛 SwiftLint
[Warning] 11-11: TODOs should be resolved (replace with Text component fo...)
(todo)
| struct Departure { | ||
| let id: String | ||
| let headsing: String | ||
| } |
There was a problem hiding this comment.
Fix typo in property name.
There's a typo in the property name 'headsing'.
struct Departure {
let id: String
- let headsing: String
+ let headsign: String
}📝 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.
| struct Departure { | |
| let id: String | |
| let headsing: String | |
| } | |
| struct Departure { | |
| let id: String | |
| let headsign: String | |
| } |
| var body: some View { | ||
| TabView { | ||
| if let departures, departures.count > 0 { | ||
| let backgroundColor = getMetroLineColor( | ||
| metroLine ?? MetroLine(rawValue: departures[0].route) | ||
| ) ?? .clear | ||
|
|
||
| PlatformDetailNextDepartureView( | ||
| headsign: departures[0].headsign, | ||
| departure: departures[0].departure.scheduled, | ||
| nextHeadsign: departures[1].headsign, | ||
| nextDeparture: departures[1].departure.predicted | ||
| ) | ||
| .containerBackground(backgroundColor.gradient, for: .tabView) | ||
|
|
||
| PlatformDetailDeparturesListView(departures: departures) | ||
| .containerBackground(backgroundColor.gradient, for: .tabView) | ||
|
|
||
| } else { | ||
| ProgressView() | ||
| } | ||
| } | ||
|
|
||
| .toolbar { | ||
| if let metroLineName = metroLine?.rawValue { | ||
| ToolbarItem( | ||
| placement: .confirmationAction) | ||
| { | ||
| Text(metroLineName) | ||
| .overlay( | ||
| Circle() | ||
| .size(width: 32, height: 32, anchor: .center) | ||
| .stroke(.white.opacity(0.6), lineWidth: 3) | ||
| ) | ||
| .fontWeight(.semibold) | ||
| .foregroundStyle(.white.opacity(0.6)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .tabViewStyle(.verticalPage(transitionStyle: .identity)) | ||
| .onAppear { | ||
| getDepartures() | ||
| } | ||
| .onReceive(timer) { _ in | ||
| getDepartures() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks and improve state handling.
The view has potential crash points and missing states:
- Unsafe array access for next departure
- No loading state between refreshes
- No error state handling
var body: some View {
TabView {
if let departures, departures.count > 0 {
let backgroundColor = getMetroLineColor(
metroLine ?? MetroLine(rawValue: departures[0].route)
) ?? .clear
PlatformDetailNextDepartureView(
headsign: departures[0].headsign,
departure: departures[0].departure.scheduled,
- nextHeadsign: departures[1].headsign,
- nextDeparture: departures[1].departure.predicted
+ nextHeadsign: departures.count > 1 ? departures[1].headsign : nil,
+ nextDeparture: departures.count > 1 ? departures[1].departure.predicted : nil
)
.containerBackground(backgroundColor.gradient, for: .tabView)Consider implementing a proper loading state and error handling:
@State private var isLoading = false
@State private var error: Error?
var body: some View {
TabView {
if let error = error {
ErrorView(error: error, retry: getDepartures)
} else if isLoading && departures == nil {
ProgressView()
} else if let departures, departures.count > 0 {
// ... existing departure views ...
} else {
Text("No departures available")
}
}
}| func getDepartures() { | ||
| NetworkManager.shared | ||
| .getDepartures(stopIds: [], platformIds: [platformId]) { result in | ||
| DispatchQueue.main.async { | ||
| switch result { | ||
| case let .success(departures): | ||
|
|
||
| self.departures = departures | ||
| print(departures) | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling and remove debug prints.
Several issues need attention:
- Empty array is passed for
stopIdsparameter - Debug print statements should be removed
- Error handling could be more robust
func getDepartures() {
NetworkManager.shared
- .getDepartures(stopIds: [], platformIds: [platformId]) { result in
+ .getDepartures(platformIds: [platformId]) { result in
DispatchQueue.main.async {
switch result {
case let .success(departures):
self.departures = departures
- print(departures)
case let .failure(error):
- print(error.localizedDescription)
+ // TODO: Implement proper error handling
+ // Consider showing an error view or alert to the user
}
}
}
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -393,15 +381,15 @@ | |||
| "$(inherited)", | |||
| "@executable_path/Frameworks", | |||
| ); | |||
| MARKETING_VERSION = 1.0; | |||
| MARKETING_VERSION = 0.2; | |||
There was a problem hiding this comment.
Version inconsistency between Watch App and main app
The Watch App's marketing version is set to 0.2 while the main app remains at 1.0. This inconsistency could cause confusion. Consider aligning the versions.
- MARKETING_VERSION = 1.0; // In main app configuration
+ MARKETING_VERSION = 0.2; // Match Watch App versionAlso applies to: 415-415, 434-434, 468-468
| IPHONEOS_DEPLOYMENT_TARGET = 17.0; | ||
| INFOPLIST_KEY_UISupportedInterfaceOrientations = UIInterfaceOrientationPortrait; | ||
| INFOPLIST_KEY_UISupportedInterfaceOrientations_iPad = "UIInterfaceOrientationLandscapeLeft UIInterfaceOrientationLandscapeRight UIInterfaceOrientationPortrait UIInterfaceOrientationPortraitUpsideDown"; | ||
| IPHONEOS_DEPLOYMENT_TARGET = 18; |
There was a problem hiding this comment.
Critical: iOS 18 deployment target is not yet released
Setting IPHONEOS_DEPLOYMENT_TARGET = 18 will prevent users from installing the app as iOS 18 is not yet released (current stable is iOS 17.x).
- IPHONEOS_DEPLOYMENT_TARGET = 18;
+ IPHONEOS_DEPLOYMENT_TARGET = 17.0;📝 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.
| IPHONEOS_DEPLOYMENT_TARGET = 18; | |
| IPHONEOS_DEPLOYMENT_TARGET = 17.0; |
| closestDistance = distance | ||
| continue | ||
| } | ||
|
|
||
| if distance < closestDistance! { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid force unwrapping to enhance safety in 'findClosestStop'
Force unwrapping closestDistance with closestDistance! can lead to runtime crashes if it is unexpectedly nil. Consider initializing closestDistance to a default value to avoid force unwrapping.
Apply this diff to refactor the function:
-func findClosestStop(to location: CLLocation, stops: [ApiStop]) -> ApiStop? {
- var closestStop: ApiStop?
- var closestDistance: CLLocationDistance?
-
- for stop in stops {
- let stopLocation = CLLocation(latitude: stop.avgLatitude, longitude: stop.avgLongitude)
-
- let distance = location.distance(from: stopLocation)
-
- guard closestDistance != nil else {
- closestStop = stop
- closestDistance = distance
- continue
- }
-
- if distance < closestDistance! {
- closestStop = stop
- closestDistance = distance
- }
- }
-
- return closestStop
-}
+func findClosestStop(to location: CLLocation, stops: [ApiStop]) -> ApiStop? {
+ var closestStop: ApiStop?
+ var closestDistance: CLLocationDistance = CLLocationDistance.greatestFiniteMagnitude
+
+ for stop in stops {
+ let stopLocation = CLLocation(latitude: stop.avgLatitude, longitude: stop.avgLongitude)
+
+ let distance = location.distance(from: stopLocation)
+
+ if distance < closestDistance {
+ closestStop = stop
+ closestDistance = distance
+ }
+ }
+
+ return closestStop
+}📝 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.
| closestDistance = distance | |
| continue | |
| } | |
| if distance < closestDistance! { | |
| func findClosestStop(to location: CLLocation, stops: [ApiStop]) -> ApiStop? { | |
| var closestStop: ApiStop? | |
| var closestDistance: CLLocationDistance = CLLocationDistance.greatestFiniteMagnitude | |
| for stop in stops { | |
| let stopLocation = CLLocation(latitude: stop.avgLatitude, longitude: stop.avgLongitude) | |
| let distance = location.distance(from: stopLocation) | |
| if distance < closestDistance { | |
| closestStop = stop | |
| closestDistance = distance | |
| } | |
| } | |
| return closestStop | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
apps/mobile/metro-now/common/components/route-name.view.swift (2)
6-9: Add documentation and default values.Consider adding documentation comments to explain the component's purpose and its properties. Also, consider providing a default background color.
+/// A view that displays a route identifier within a colored badge +/// Used for showing metro/bus line numbers or letters struct RouteNameIconView: View { + /// The text to display within the badge (e.g., line number or letter) let label: String + /// The background color of the badge - let background: Color + let background: Color = .gray
21-56: Enhance preview with edge cases and better organization.While the current preview is good, consider enhancing it with:
- Edge cases (very long route numbers)
- Grid layout for better visualization
- Dark/light mode testing
#Preview { + Grid { + GridRow { RouteNameIconView( label: "a", background: .green ) RouteNameIconView( label: "b", background: .yellow ) RouteNameIconView( label: "c", background: .red ) + } + GridRow { RouteNameIconView( label: "28", background: .purple ) RouteNameIconView( label: "99", background: .black ) RouteNameIconView( label: "149", background: .blue ) + } + GridRow { RouteNameIconView( label: "912", background: .black ) + // Test long route number + RouteNameIconView( + label: "1234", + background: .orange + ) + } + } + .preferredColorScheme(.dark) + .padding()apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page.view.swift (2)
6-9: Add documentation and access modifiers.Consider adding documentation to explain the view's purpose and its properties. Also, explicitly specify access modifiers for better encapsulation.
+/// A view that displays platforms and departures for the closest stop. struct ClosestStopPageView: View { + /// The closest stop containing platform information. - let closestStop: ApiStop + private let closestStop: ApiStop + /// Optional array of departures for the platforms. - let departures: [ApiDeparture]? + private let departures: [ApiDeparture]?
10-34: Consider extracting view builders for better maintainability.The
bodyproperty contains complex conditional rendering logic. Consider extracting this into separate view builder methods for better maintainability and testability.+private func buildDepartureView( + platformDepartures: [ApiDeparture], + routeLabel: String, + routeLabelBackground: Color +) -> some View { + ClosestStopPageListItemView( + routeLabel: routeLabel, + routeLabelBackground: routeLabelBackground, + headsign: platformDepartures[0].headsign, + departure: platformDepartures[0].departure.predicted, + nextHeadsign: platformDepartures.count > 1 ? platformDepartures[1].headsign : nil, + nextDeparture: platformDepartures.count > 1 ? platformDepartures[1].departure.scheduled : nil + ) +} var body: some View { List(closestStop.platforms, id: \.id) { platform in // ... route label and background setup ... if let platformDepartures, !platformDepartures.isEmpty { - ClosestStopPageListItemView(...) + buildDepartureView( + platformDepartures: platformDepartures, + routeLabel: routeLabel, + routeLabelBackground: routeLabelBackground + ) } else { ClosestStopPageListItemPlaceholderView(...) } } }apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item-placeholder.view.swift (4)
1-4: Consider enhancing file documentation.While the basic header with repository link is helpful, consider adding:
- Copyright notice
- Brief file purpose documentation
- Usage examples
// metro-now // https://github.com/krystxf/metro-now +// Copyright © 2024 krystxf. All rights reserved. + +/// A placeholder view for displaying loading state of closest stop list items. +/// Usage: +/// ```swift +/// ClosestStopPageListItemPlaceholderView( +/// routeLabel: "A", +/// routeLabelBackground: .green +/// ) +/// ```
6-9: Add documentation for the view struct and its properties.Consider adding documentation comments to improve code maintainability.
+/// A placeholder view that displays a loading state for a closest stop list item. struct ClosestStopPageListItemPlaceholderView: View { + /// The label text for the route (e.g., "A", "C"). let routeLabel: String + /// The background color for the route label. let routeLabelBackground: Color
14-17: Improve string localization and time calculations.The current implementation has several potential improvements:
- Hardcoded strings should be localized
- Magic numbers in time calculations should be constants
+ private static let loadingText = NSLocalizedString("Loading...", comment: "Placeholder text for loading state") + private static let initialDepartureOffset: TimeInterval = 10 * 60 // 10 minutes + private static let nextDepartureOffset: TimeInterval = 15 * 60 // 15 minutes + var body: some View { ClosestStopPageListItemView( routeLabel: routeLabel, routeLabelBackground: routeLabelBackground, - headsign: "Loading...", - departure: .now + 10 * 60, - nextHeadsign: "Loading..", - nextDeparture: .now + 15 * 60 + headsign: Self.loadingText, + departure: .now + Self.initialDepartureOffset, + nextHeadsign: Self.loadingText, + nextDeparture: .now + Self.nextDepartureOffset
23-63: Improve preview configurations.There are several issues with the preview configurations:
- Typo in the first preview name ("signle" should be "single")
- Code duplication between previews
- Missing documentation for preview configurations
Consider refactoring to reduce duplication:
-#Preview("signle metro line") { +/// Preview helper to create a list of placeholder items +private struct PreviewList: View { + let items: [(label: String, color: Color)] + + var body: some View { + List { + ForEach(items, id: \.label) { item in + ClosestStopPageListItemPlaceholderView( + routeLabel: item.label, + routeLabelBackground: item.color + ) + } + } + } +} + +#Preview("single metro line") { + PreviewList(items: [ + ("A", .green), + ("A", .green), + ("C", .red), + ("C", .red) + ]) +} + +#Preview("multiple metro lines") { + PreviewList(items: [ + ("A", .green), + ("A", .green), + ("C", .red), + ("C", .red) + ])apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item.view.swift (4)
1-3: Add comprehensive file header documentation.Consider adding a more detailed file header that includes:
- Copyright notice
- Brief description of the view's purpose and functionality
- Any important usage notes or requirements
-// metro-now -// https://github.com/krystxf/metro-now +// +// ClosestStopPageListItemView.swift +// metro-now +// +// Created by <author> on <date> +// Copyright © 2024 metro-now. All rights reserved. +// +// A view component that displays transit stop information including +// route label, destination, and departure times in a list item format.
6-14: Add property documentation and explicit types.Properties would benefit from documentation comments and explicit type annotations for better code clarity and maintainability.
struct ClosestStopPageListItemView: View { + /// The label/identifier of the transit route (e.g., "A", "B") let routeLabel: String + /// The background color for the route label. Defaults to black if nil let routeLabelBackground: Color? + /// The destination or direction of the transit route let headsign: String + /// The scheduled departure time let departure: Date + /// The destination of the next departure, if different let nextHeadsign: String? + /// The scheduled time for the next departure let nextDeparture: Date?
16-52: Enhance view implementation with accessibility and constants.The view implementation is well-structured but could benefit from the following improvements:
- Extract magic numbers into named constants:
+ private enum Constants { + static let stackSpacing: CGFloat = 8 + static let innerSpacing: CGFloat = 4 + } var body: some View { HStack( alignment: .top, - spacing: 8 + spacing: Constants.stackSpacing
- Add accessibility support:
HStack { Text(headsign) + .accessibilityLabel("Destination: \(headsign)") Spacer() CountdownView(targetDate: departure) + .accessibilityLabel("Departing in") }
- Consider extracting the next departure section into a separate view for better maintainability:
private struct NextDepartureRow: View { let currentHeadsign: String let nextHeadsign: String let nextDeparture: Date var body: some View { HStack { if currentHeadsign != nextHeadsign { Text(nextHeadsign) } Spacer() CountdownView(targetDate: nextDeparture) { currentHeadsign == nextHeadsign ? $0 : "Also in \($0)" } } .foregroundStyle(.secondary) .font(.footnote) } }
54-73: Enhance preview with additional test cases.The current preview is good but could be more comprehensive to help visualize different states and appearances.
-#Preview("single metro line") { +#Preview("Metro Line Previews") { List { + Group { + Text("Standard Cases") + .font(.headline) + .listRowBackground(Color.clear) + ClosestStopPageListItemView( routeLabel: "A", routeLabelBackground: .green, headsign: "Nemocnice Motol", departure: .now + 10 * 60, nextHeadsign: "Nemocnice Motol", nextDeparture: .now + 15 * 60 ) + ClosestStopPageListItemView( routeLabel: "A", routeLabelBackground: .green, headsign: "Depo Hostivař", departure: .now + 10 * 60, nextHeadsign: "Skalka", nextDeparture: .now + 15 * 60 ) + + Text("Edge Cases") + .font(.headline) + .listRowBackground(Color.clear) + + // Without next departure + ClosestStopPageListItemView( + routeLabel: "B", + routeLabelBackground: .yellow, + headsign: "Černý Most", + departure: .now + 5 * 60, + nextHeadsign: nil, + nextDeparture: nil + ) + } } + .preferredColorScheme(.light) + + List { + // Same previews with dark mode + // ... (repeat the above items) + } + .preferredColorScheme(.dark) }apps/mobile/metro-now/metro-now/ContentView.swift (2)
9-10: Remove redundant nil initializationThe explicit nil initialization is redundant as optional properties are automatically initialized to nil.
- @State var stops: [ApiStop]? = nil - @State var departures: [ApiDeparture]? = nil + @State var stops: [ApiStop]? + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
11-11: Consider increasing the polling intervalA 2-second polling interval for departure updates might be too aggressive and could impact battery life. Consider increasing it to 30-60 seconds unless there's a specific requirement for such frequent updates.
- private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect() + private let timer = Timer.publish(every: 30, on: .main, in: .common).autoconnect()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/mobile/metro-now/common/components/route-name.view.swift(1 hunks)apps/mobile/metro-now/common/utils/metro-line.utils.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift(2 hunks)apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj(13 hunks)apps/mobile/metro-now/metro-now/ContentView.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item-placeholder.view.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item.view.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page.view.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mobile/metro-now/common/utils/metro-line.utils.swift
- apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift
🧰 Additional context used
🪛 SwiftLint
apps/mobile/metro-now/metro-now/ContentView.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (11)
apps/mobile/metro-now/common/components/route-name.view.swift (1)
1-4: LGTM! Good documentation practice.
The file header with the repository URL provides helpful context for developers.
apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page.view.swift (2)
1-5: LGTM! Good header documentation.
The file header with repository URL and necessary import statement are well structured.
18-27:
Handle edge cases in departure display.
The code assumes there are at least two departures when showing departure information. This could crash if there's only one departure.
- if let platformDepartures, platformDepartures.count > 0 {
+ if let platformDepartures, !platformDepartures.isEmpty {
ClosestStopPageListItemView(
routeLabel: routeLabel,
routeLabelBackground: routeLabelBackground,
headsign: platformDepartures[0].headsign,
departure: platformDepartures[0].departure.predicted,
- nextHeadsign: platformDepartures[1].headsign,
- nextDeparture: platformDepartures[1].departure.scheduled
+ nextHeadsign: platformDepartures.count > 1 ? platformDepartures[1].headsign : nil,
+ nextDeparture: platformDepartures.count > 1 ? platformDepartures[1].departure.scheduled : nil
)Note: This assumes ClosestStopPageListItemView has been updated to handle optional nextHeadsign and nextDeparture.
apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj (8)
47-47: LGTM: Component file exception added correctly.
The route-name.view.swift file has been properly added to the Watch App target exceptions.
176-176: LGTM: Project upgrade check version updated.
The LastUpgradeCheck has been updated to 1610, which is appropriate for the current Xcode version.
283-283: LGTM: Dead code stripping enabled.
Enabling DEAD_CODE_STRIPPING is a good optimization that helps reduce the final binary size by removing unused code.
Also applies to: 345-345
371-371: Version bump looks good.
The CURRENT_PROJECT_VERSION has been consistently updated to 30 across all configurations.
Also applies to: 403-403, 436-436, 470-470
385-385: Version alignment achieved.
The MARKETING_VERSION has been consistently set to 0.2 across all targets, addressing the previous version inconsistency between Watch App and main app.
Also applies to: 417-417, 454-454, 488-488
393-393: LGTM: watchOS deployment target update.
The WATCHOS_DEPLOYMENT_TARGET has been set to 11, which is a reasonable minimum version that maintains good device compatibility while allowing access to newer APIs.
Also applies to: 426-426
447-448: LGTM: Interface orientations properly configured.
The interface orientations are well-configured:
- iPhone: Portrait only
- iPad: All orientations supported (both landscape and portrait)
Also applies to: 481-482
449-449:
Critical: iOS 18 deployment target is not yet released.
Setting IPHONEOS_DEPLOYMENT_TARGET to 18 will prevent users from installing the app as iOS 18 is not yet released (current stable is iOS 17.x).
Apply this diff:
- IPHONEOS_DEPLOYMENT_TARGET = 18;
+ IPHONEOS_DEPLOYMENT_TARGET = 17.0;Also applies to: 483-483
| var body: some View { | ||
| Text(label.uppercased()) | ||
| .font(.system(size: 12, weight: .bold)) | ||
| .foregroundStyle(.white) | ||
| .fixedSize(horizontal: true, vertical: true) | ||
| .frame(width: 26, height: 26) | ||
| .background(Rectangle().fill(background)) | ||
| .clipShape(.rect(cornerRadius: 6)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider accessibility and dynamic sizing.
The current implementation has several potential improvements:
- Add accessibility label for screen readers
- Consider dynamic sizing for longer route numbers
- Add minimum contrast ratio verification for background colors
var body: some View {
Text(label.uppercased())
.font(.system(size: 12, weight: .bold))
.foregroundStyle(.white)
- .fixedSize(horizontal: true, vertical: true)
- .frame(width: 26, height: 26)
+ .frame(minWidth: 26, minHeight: 26)
+ .padding(.horizontal, 4)
.background(Rectangle().fill(background))
.clipShape(.rect(cornerRadius: 6))
+ .accessibilityLabel("Route \(label)")
}Committable suggestion skipped: line range outside the PR's diff.
| List(closestStop.platforms, id: \.id) { platform in | ||
| let routeLabel: String = platform.routes[0].name | ||
| let routeLabelBackground: Color = getMetroLineColor(routeLabel) ?? .black | ||
| let platformDepartures: [ApiDeparture]? = departures?.filter { departure in | ||
| departure.platformId == platform.id | ||
| } | ||
|
|
There was a problem hiding this comment.
Add safety checks for platform routes.
The code assumes each platform has at least one route, which could lead to a crash. Also, the filtering logic could be extracted for better readability.
List(closestStop.platforms, id: \.id) { platform in
- let routeLabel: String = platform.routes[0].name
+ guard let firstRoute = platform.routes.first else {
+ return EmptyView()
+ }
+ let routeLabel: String = firstRoute.name
let routeLabelBackground: Color = getMetroLineColor(routeLabel) ?? .black
- let platformDepartures: [ApiDeparture]? = departures?.filter { departure in
- departure.platformId == platform.id
- }
+ let platformDepartures = filterDeparturesForPlatform(platform.id)
}
+private func filterDeparturesForPlatform(_ platformId: String) -> [ApiDeparture]? {
+ return departures?.filter { $0.platformId == platformId }
+}Committable suggestion skipped: line range outside the PR's diff.
| } else { | ||
| ProgressView() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error state handling
The current implementation only shows a loading indicator when data is unavailable. Consider adding error states to inform users when location services are disabled or network requests fail.
} else {
- ProgressView()
+ VStack {
+ if locationManager.authorizationStatus == .denied {
+ Text("Please enable location services")
+ .foregroundColor(.secondary)
+ } else if stops == nil {
+ ProgressView()
+ Text("Loading stops...")
+ .foregroundColor(.secondary)
+ } else {
+ ProgressView()
+ Text("Finding nearest stop...")
+ .foregroundColor(.secondary)
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| func getAllMetroStops() { | ||
| NetworkManager.shared.getMetroStops { result in | ||
| DispatchQueue.main.async { | ||
| self.location = location | ||
| switch result { | ||
| case let .success(stops): | ||
|
|
||
| self.stops = stops | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in getAllMetroStops
Currently, network errors are only printed to the console. Consider implementing proper error handling to inform users and potentially retry the request.
func getAllMetroStops() {
+ @State private var hasError = false
+
NetworkManager.shared.getMetroStops { result in
DispatchQueue.main.async {
switch result {
case let .success(stops):
self.stops = stops
+ self.hasError = false
case let .failure(error):
- print(error.localizedDescription)
+ self.hasError = true
+ // Retry after delay
+ DispatchQueue.main.asyncAfter(deadline: .now() + 5) {
+ self.getAllMetroStops()
+ }
}
}
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/backend/src/modules/departure/departure.controller.ts (1)
Line range hint
43-106: Consider refactoring for better code reuse and type safetyThe endpoints share similar logic but have duplicated validation and service calls. Consider these improvements:
- Extract common validation logic:
// Create a shared validator const validateIds = (ids: unknown[], type: 'platform' | 'stop') => { const schema = z.string().array() .min(1) .max(QUERY_IDS_COUNT_MAX); const parsed = schema.safeParse(toArray(ids)); if (!parsed.success) { throw new HttpException( `Invalid ${type} IDs provided`, HttpStatus.BAD_REQUEST, ); } return parsed.data; };
- Type the query parameters:
interface DepartureQuery { metroOnly?: boolean; platform?: string[]; stop?: string[]; }These changes would improve maintainability and reduce potential bugs from duplicated logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/backend/src/config/cache-module.config.ts(0 hunks)apps/backend/src/modules/departure/departure.controller.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/backend/src/config/cache-module.config.ts
🔇 Additional comments (1)
apps/backend/src/modules/departure/departure.controller.ts (1)
28-28: Review cache duration increase and memory implications
The cache TTL has been doubled from 2 seconds to 4 seconds. While this reduces backend load, please consider:
- For metro departures, 4 seconds of data staleness might impact user experience, especially for real-time updates
- Combined with the removal of max cache items limit (from cache-module.config.ts), this could lead to increased memory usage over time
Could you verify:
- The acceptable staleness threshold for departure data?
- Whether memory monitoring is in place?
Consider implementing:
- Different cache durations based on time-to-departure (shorter TTL for imminent departures)
- Memory usage monitoring and cache eviction strategies
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (4)
11-15: Add documentation for MainPagePlatform struct.Consider adding documentation comments to explain the purpose of this structure and its properties, especially the relationship between metroLine and departures being optional.
+/// Represents a metro platform with its associated line and departure information struct MainPagePlatform { + /// Unique identifier for the platform let id: String + /// The metro line associated with this platform, if any let metroLine: MetroLine? + /// List of departures from this platform, if available let departures: [ApiDeparture]? }
21-21: Remove redundant nil initialization.The explicit nil initialization is unnecessary as optional properties are nil by default in Swift.
- @State var selectedPlatformId: String? = nil + @State var selectedPlatformId: String?🧰 Tools
🪛 SwiftLint
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
28-38: Simplify departure rendering logic.The nested conditional logic could be more readable by extracting the departure view logic into a separate view component.
+private struct DepartureItemView: View { + let departures: [ApiDeparture] + let itemColor: Color + + var body: some View { + let hasNextDeparture = departures.count > 1 + return StopDepartureListItemView( + color: itemColor, + headsign: departures[0].headsign, + departure: departures[0].departure.predicted, + nextHeadsign: hasNextDeparture ? departures[1].headsign : nil, + nextDeparture: hasNextDeparture ? departures[1].departure.predicted : nil + ) + } +} // In the List view: - if let departures = platform.departures, departures.count > 0 { - let hasNextDeparture = departures.count > 1 - - StopDepartureListItemView( - color: itemColor, - headsign: departures[0].headsign, - departure: departures[0].departure.predicted, - nextHeadsign: hasNextDeparture ? departures[1].headsign : nil, - nextDeparture: hasNextDeparture ? departures[1].departure.predicted : nil - ) + if let departures = platform.departures, !departures.isEmpty { + DepartureItemView(departures: departures, itemColor: itemColor)
68-94: Enhance preview with sample departure data.Consider adding a second preview with sample departure data to better visualize how the view handles actual departures.
#Preview { + Group { StopDeparturesView( title: "Florenc", platforms: [ MainPagePlatform( id: "U689Z101P", metroLine: .B, departures: nil ), // ... other platforms ... ] ) + + StopDeparturesView( + title: "Můstek", + platforms: [ + MainPagePlatform( + id: "U123Z101P", + metroLine: .A, + departures: [ + ApiDeparture(headsign: "Nemocnice Motol", departure: Departure(predicted: Date())), + ApiDeparture(headsign: "Depo Hostivař", departure: Departure(predicted: Date().addingTimeInterval(180))) + ] + ) + ] + ) + } }apps/mobile/metro-now/metro-now/ContentView.swift (1)
8-10: Remove redundant nil initializationThe explicit nil initialization is redundant for optional variables. Swift automatically initializes them as nil.
- @State var stops: [ApiStop]? = nil - @State var departures: [ApiDeparture]? = nil + @State var stops: [ApiStop]? + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift (2)
6-11: Consider increasing the refresh interval.A 2-second refresh interval might be too aggressive for a watch app, potentially impacting battery life and network usage. Consider increasing it to 5-10 seconds unless there's a specific requirement for real-time updates.
- private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect() + private let timer = Timer.publish(every: 5, on: .main, in: .common).autoconnect()Also, the optional departures property has a redundant nil initialization:
- @State var departures: [ApiDeparture]? = nil + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
90-97: Consider adding previews for different states.The current preview only shows the ideal state. Consider adding previews for loading, error, and empty states to help with UI development.
#Preview("Loading State") { NavigationStack { PlatformDetailView( platformId: "U1040Z101P", metroLine: MetroLine.B, departures: nil ) } } #Preview("Empty State") { NavigationStack { PlatformDetailView( platformId: "U1040Z101P", metroLine: MetroLine.B, departures: [] ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
assets/screenshots/iOS/closest-stop-departures-list.pngis excluded by!**/*.pngassets/screenshots/iPadOS/closest-stop-departures-list.pngis excluded by!**/*.png
📒 Files selected for processing (18)
apps/backend/src/config/cache-module.config.ts(0 hunks)apps/backend/src/modules/departure/departure.controller.ts(1 hunks)apps/mobile/metro-now/common/components/route-name.view.swift(1 hunks)apps/mobile/metro-now/common/utils/metro-line.utils.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/ContentView.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/departure-placeholder.swift(0 hunks)apps/mobile/metro-now/metro-now Watch App/pages/main-page.swift(0 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/next-departure.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-departures-list.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item-placeholder.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift(2 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj(13 hunks)apps/mobile/metro-now/metro-now/ContentView.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item-placeholder.view.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item.view.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page.view.swift(1 hunks)
💤 Files with no reviewable changes (3)
- apps/backend/src/config/cache-module.config.ts
- apps/mobile/metro-now/metro-now Watch App/pages/departure-placeholder.swift
- apps/mobile/metro-now/metro-now Watch App/pages/main-page.swift
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/backend/src/modules/departure/departure.controller.ts
- apps/mobile/metro-now/common/components/route-name.view.swift
- apps/mobile/metro-now/common/utils/metro-line.utils.swift
- apps/mobile/metro-now/metro-now Watch App/ContentView.swift
- apps/mobile/metro-now/metro-now Watch App/pages/platform/next-departure.view.swift
- apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-departures-list.view.swift
- apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item-placeholder.view.swift
- apps/mobile/metro-now/metro-now Watch App/pages/stop/list-item.view.swift
- apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item-placeholder.view.swift
- apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page-list-item.view.swift
- apps/mobile/metro-now/metro-now/pages/closest-stop/closest-stop-page.view.swift
🧰 Additional context used
🪛 SwiftLint
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now/ContentView.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (15)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (1)
6-9: Fix typo in property name 'headsing'.
apps/mobile/metro-now/metro-now/ContentView.swift (2)
13-34: LGTM with navigation structure
The NavigationStack implementation and conditional rendering look good. Note that there's a previous review comment about enhancing error state handling that's still applicable.
36-57: Note existing review for force unwrapping
The implementation contains force unwrapping that could lead to crashes. Please refer to the previous review comment for the suggested fix.
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift (5)
1-5: LGTM!
The file header and imports are appropriate for a SwiftUI view.
12-21: LGTM!
The initializer is well-structured with appropriate optional parameters.
23-70: Previous review comments about array safety and error handling are still applicable.
The unsafe array access and missing error states need to be addressed as mentioned in the previous review.
46-61: LGTM! Well-implemented toolbar.
The toolbar implementation with the metro line indicator is visually appealing and follows good SwiftUI practices.
72-87: Previous review comments about network call implementation are still applicable.
The issues with empty stopIds array and debug print statements need to be addressed as mentioned in the previous review.
apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj (7)
47-47: LGTM: Route name view integration
The addition of "components/route-name.view.swift" to the Watch App target's exceptions is correctly configured.
176-176: LGTM: Project upgrade
The LastUpgradeCheck has been updated from 1600 to 1610, keeping the project up to date with the latest Xcode version.
283-283: Performance improvement: Dead code stripping enabled
The addition of DEAD_CODE_STRIPPING = YES in both Debug and Release configurations will help reduce the final binary size by removing unused code.
Also applies to: 345-345
449-449: Critical: iOS 18 deployment target is not yet released
Also applies to: 483-483
447-448: LGTM: Interface orientation settings
The interface orientation settings are well-configured:
- iPhone: Portrait only (appropriate for a metro app)
- iPad: All orientations supported (provides flexibility for tablet users)
Also applies to: 481-482
371-371: Version management needs attention
The current configuration shows:
- Current project version: 30
- Marketing version: 0.2
Consider implementing semantic versioning (MAJOR.MINOR.PATCH) and ensure version numbers reflect the app's maturity level. The current combination of a high build number (30) with a low marketing version (0.2) might indicate:
- Either the marketing version should be increased to reflect the maturity
- Or the build number should be reset to align with the early stage version
Also applies to: 385-385, 403-403, 417-417, 436-436, 454-454, 470-470, 488-488
✅ Verification successful
The verification results confirm the original review comment's findings. I can now generate the final response since I have sufficient information about version numbers across all targets.
Version numbers need alignment across the project
The verification confirms consistent but potentially misaligned version numbers:
- Marketing version (0.2) is consistently set across all targets
- Build number (30) is consistently set across all targets
This combination suggests early-stage software (0.2) with multiple iterations (30 builds), which is valid but warrants consideration:
- Consider if 0.2 accurately reflects the app's current feature completeness
- If maintaining 0.2, consider resetting build numbers for the next minor version
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check version numbers across targets
echo "Checking version consistency across targets..."
rg "MARKETING_VERSION = \d+\.\d+" -A 1
rg "CURRENT_PROJECT_VERSION = \d+" -A 1
Length of output: 1907
393-393: Verify watchOS deployment target compatibility
The watchOS deployment target has been set to 11, which is quite high. This might limit the app's reach to users with older Apple Watches.
Consider lowering the deployment target if supporting older devices is important for your user base.
Also applies to: 426-426
✅ Verification successful
Let me try to gather more context about the watchOS deployment target from the codebase.
Let me try one more time with corrected commands:
Based on the project configuration, I can now provide a final response:
watchOS 11 requirement is appropriate for this project
The watchOS deployment target of 11.0 is justified for this project because:
- The app is using SwiftUI and modern iOS/watchOS features (indicated by ENABLE_PREVIEWS setting)
- The companion iOS app targets iOS 18, suggesting this is a modern app targeting recent OS versions
- The project was created with Xcode 16.0 (LastSwiftUpdateCheck = 1600) and upgraded to 16.1
- The app is still in early development (version 0.2) and not yet widely distributed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check watchOS version distribution stats
echo "Note: Please verify the watchOS version distribution in App Store Connect"
echo "and consider if watchOS 11 requirement aligns with your target audience."
Length of output: 311
Script:
#!/bin/bash
# Check for any SwiftUI features or APIs that might require watchOS 11
rg -i "import.*watchkit|import.*watchconnectivity" -A 5
# Look for any watchOS specific code or configurations
rg -i "watchos" --type xml --type swift --type objc
# Check project configuration files for any watchOS related settings
fd "project.pbxproj|*.xcconfig" --type f --exec cat {}
Length of output: 490
Script:
#!/bin/bash
# Search for watchOS related imports and features
rg -i "import.*watchkit|import.*watchconnectivity" -A 5
# Look for watchOS specific configurations
fd -e pbxproj -e xcconfig --exec cat {}
# Check for any SwiftUI features that might require watchOS 11
rg -i "\.watchos|watchos" --type swift --type objc --type xml
Length of output: 19551
| let platform = platforms.first( | ||
| where: { | ||
| $0.id == selectedPlatformId | ||
|
|
||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize platform lookup performance.
The first(where:) lookup is performed on every view update. Consider storing the selected platform in a computed property or moving the lookup logic outside the view body.
struct StopDeparturesView: View {
// ... existing properties ...
+
+ private var selectedPlatform: MainPagePlatform? {
+ platforms.first { $0.id == selectedPlatformId }
+ }
var body: some View {
NavigationSplitView {
// ... list view ...
} detail: {
- let platform = platforms.first(
- where: {
- $0.id == selectedPlatformId
- })
-
- if let platform {
+ if let platform = selectedPlatform {
PlatformDetailView(📝 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.
| let platform = platforms.first( | |
| where: { | |
| $0.id == selectedPlatformId | |
| }) | |
| private var selectedPlatform: MainPagePlatform? { | |
| platforms.first { $0.id == selectedPlatformId } | |
| } | |
| var body: some View { | |
| NavigationSplitView { | |
| // ... list view ... | |
| } detail: { | |
| if let platform = selectedPlatform { | |
| PlatformDetailView( |
| @StateObject private var locationManager = LocationManager() | ||
| @State var stops: [ApiStop]? = nil | ||
| @State var departures: [ApiDeparture]? = nil | ||
| private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider increasing the timer interval
A 2-second interval for API calls might be too frequent and could:
- Increase server load
- Drain device battery
- Potentially hit API rate limits
Consider increasing the interval to 30-60 seconds for departure updates.
- private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect()
+ private let timer = Timer.publish(every: 30, on: .main, in: .common).autoconnect()📝 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.
| private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect() | |
| private let timer = Timer.publish(every: 30, on: .main, in: .common).autoconnect() |
| func getStopDepartures() { | ||
| guard | ||
| let location = locationManager.location, | ||
| let stops, | ||
| let closestStop = findClosestStop(to: location, stops: stops) | ||
| else { | ||
| return | ||
| } | ||
|
|
||
| var body: some View { | ||
| VStack { | ||
| if let location = locationManager.location { | ||
| Text("Latitude: \(location.coordinate.latitude)") | ||
| Text("Longitude: \(location.coordinate.longitude)") | ||
| } else { | ||
| Text("Fetching location...") | ||
| NetworkManager.shared | ||
| .getDepartures(stopIds: [closestStop.id], platformIds: []) { result in | ||
| DispatchQueue.main.async { | ||
| switch result { | ||
| case let .success(departures): | ||
|
|
||
| self.departures = departures | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for network calls
The view should cancel ongoing network requests when it disappears to prevent memory leaks and unnecessary network traffic.
struct ContentView: View {
+ private var cancellables = Set<AnyCancellable>()
+
// ... existing code ...
var body: some View {
NavigationStack {
// ... existing code ...
}
.onAppear {
getAllMetroStops()
}
.onReceive(timer) { _ in
getStopDepartures()
}
+ .onDisappear {
+ cancellables.forEach { $0.cancel() }
+ }
}
func getStopDepartures() {
// ... existing code ...
NetworkManager.shared
.getDepartures(stopIds: [closestStop.id], platformIds: []) { result in
// ... existing code ...
}
+ .store(in: &cancellables)
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (17)
apps/mobile/metro-now/common/utils/get-color-by-route-name.swift (3)
1-7: Add documentation to improve code maintainability.Consider adding documentation comments that explain:
- The purpose of this utility file
- The transport types supported (metro, tram, bus, ferry, funicular)
- The color mapping logic and its business rules
// metro-now // https://github.com/krystxf/metro-now + +/// Utility for determining colors based on public transport route identifiers. +/// Supports various transport types: +/// - Metro lines (A, B, C) +/// - Trams (numbers < 100) +/// - Buses (numbers >= 100) +/// - Ferry (P prefix) +/// - Funicular (LD prefix)
8-18: Improve metro line color mapping implementation.The function could benefit from:
- Documentation explaining the color mapping rationale
- Using
@unknown defaultfor better type safety+/// Returns the official color for a given metro line. +/// - Parameter metroLine: The metro line identifier +/// - Returns: The official color for the line, or black if unknown func getColorByRouteName(_ metroLine: MetroLine?) -> Color { switch metroLine { case .A: .green case .B: .yellow case .C: .red - default: FALLBACK_COLOR + @unknown default: FALLBACK_COLOR } }
1-64: Consider architectural improvements for better maintainability and flexibility.
The current implementation could benefit from a more structured approach to color management:
- Consider creating a dedicated color palette enum/struct
- Move transport type definitions to a separate file
- Add validation for edge cases (empty strings, malformed input)
Some transport types share the same color (ferry and funicular both use blue), which might cause confusion in the UI. Consider using distinct colors or adding secondary visual indicators.
Would you like me to provide a detailed example of these architectural improvements?
apps/mobile/metro-now/metro-now/pages/closest-stop/closest-metro-stop-section.view.swift (5)
1-6: Remove unnecessary empty lineimport SwiftUI -
7-9: Add documentation for the viewConsider adding a documentation comment explaining the view's purpose and its parameters.
+/// A view that displays departure information for the closest metro stop +/// - Parameters: +/// - closestStop: The closest stop information containing platforms and routes +/// - departures: Optional array of departures for the stop struct ClosestMetroStopSectionView: View { let closestStop: ApiStop let departures: [ApiDeparture]?
15-17: Simplify departure filtering logicThe closure syntax can be simplified using method reference.
- let platformDepartures: [ApiDeparture]? = departures?.filter { departure in - departure.platformId == platform.id - } + let platformDepartures: [ApiDeparture]? = departures?.filter(\.platformId == platform.id)
43-46: Remove debug print statementThe print statement should be removed before production deployment.
init(departures: [ApiDeparture]) { self.departures = departures - print(departures.count) }
40-60: Consider handling empty departures arrayThe view doesn't handle the case when departures array is empty. Consider showing a message or placeholder.
var body: some View { + if departures.isEmpty { + Text("No departures available") + .foregroundColor(.secondary) + } else { ForEach(departures, id: \.headsign) { departure in // ... existing code ... } + } }apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (3)
21-21: Remove redundant nil initialization.The explicit nil initialization is redundant for an optional property.
- @State var selectedPlatformId: String? = nil + @State var selectedPlatformId: String?🧰 Tools
🪛 SwiftLint
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
23-65: Consider extracting view components for better maintainability.The body contains complex conditional logic that could be extracted into separate view components:
- The platform list item view logic (lines 28-43)
- The detail view logic (lines 55-63)
This would improve readability and make the code more maintainable.
Example structure:
private struct PlatformListItem: View { let platform: MainPagePlatform var body: some View { // Extract list item logic here } } private struct DetailContent: View { let platform: MainPagePlatform? var body: some View { // Extract detail view logic here } }
68-94: Enhance preview with realistic sample data.The current preview uses nil for all departures. Consider adding sample departure data to better represent the actual UI states.
Example:
#Preview { StopDeparturesView( title: "Florenc", platforms: [ MainPagePlatform( id: "U689Z101P", metroLine: .B, departures: [ ApiDeparture( headsign: "Zličín", departure: Departure(predicted: Date()) ) ] ), // ... other platforms ] ) }apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift (3)
9-9: Simplify optional property declaration.The explicit nil initialization is redundant as optional properties are automatically initialized to nil.
- @State var departures: [ApiDeparture]? = nil + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
10-10: Consider increasing the refresh interval.A 2-second refresh interval might be too aggressive and could:
- Impact battery life on the watch
- Generate unnecessary load on the backend
- Lead to rate limiting issues
Consider increasing it to 30 seconds or 1 minute, which should still provide timely updates for metro schedules.
- private let timer = Timer.publish(every: 2, on: .main, in: .common).autoconnect() + private let timer = Timer.publish(every: 30, on: .main, in: .common).autoconnect()
89-96: Enhance preview configurations.Consider adding multiple preview configurations to visualize different states:
- Loading state
- Error state
- Empty departures
- Multiple departures
#Preview("Loading") { NavigationStack { PlatformDetailView( platformId: "U1040Z101P", metroLine: MetroLine.B, departures: nil ) } } #Preview("With Departures") { NavigationStack { PlatformDetailView( platformId: "U1040Z101P", metroLine: MetroLine.B, departures: [ // Add sample departures here ] ) } }apps/mobile/metro-now/metro-now.xcodeproj/xcshareddata/xcschemes/metro-now Watch App.xcscheme (1)
40-46: Consider configuring a test planThe scheme is set to auto-create a test plan (
shouldAutocreateTestPlan = "YES"). While this is acceptable, consider creating a custom test plan to:
- Define specific test configurations
- Set up test coverage requirements
- Configure parallel testing options
apps/mobile/metro-now/metro-now/ContentView.swift (2)
9-11: Simplify state declarations by removing redundant nil initialization.The explicit nil initialization is redundant for optional state variables.
- @State var metroStops: [ApiStop]? = nil - @State var allStops: [ApiStop]? = nil - @State var departures: [ApiDeparture]? = nil + @State var metroStops: [ApiStop]? + @State var allStops: [ApiStop]? + @State var departures: [ApiDeparture]?🧰 Tools
🪛 SwiftLint
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 11-11: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
31-59: Simplify complex nested conditional rendering.The nested conditional rendering with multiple optional bindings makes the code harder to maintain. Consider extracting the non-metro platforms section into a separate view component.
struct NonMetroPlatformsView: View { let stops: [ApiStop] let location: CLLocation let departures: [ApiDeparture]? var body: some View { if let closestStop = findClosestStop(to: location, stops: stops) { ForEach(Dictionary(grouping: closestStop.platforms.filter { !$0.isMetro }, by: \.name) .sorted { $0.key < $1.key }, id: \.key) { groupName, groupPlatforms in Section(groupName) { ForEach(groupPlatforms) { platform in PlatformDeparturesView( departures: departures?.filter { $0.platformId == platform.id } ?? [] ) } } } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
apps/mobile/metro-now/common/const/api-const.swift(1 hunks)apps/mobile/metro-now/common/managers/network-manager.swift(3 hunks)apps/mobile/metro-now/common/utils/get-color-by-route-name.swift(1 hunks)apps/mobile/metro-now/common/utils/metro-line.utils.swift(0 hunks)apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift(1 hunks)apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj(14 hunks)apps/mobile/metro-now/metro-now.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings(1 hunks)apps/mobile/metro-now/metro-now.xcodeproj/xcshareddata/xcschemes/metro-now Watch App.xcscheme(1 hunks)apps/mobile/metro-now/metro-now.xcodeproj/xcshareddata/xcschemes/metro-now.xcscheme(1 hunks)apps/mobile/metro-now/metro-now/ContentView.swift(1 hunks)apps/mobile/metro-now/metro-now/pages/closest-stop/closest-metro-stop-section.view.swift(1 hunks)
💤 Files with no reviewable changes (1)
- apps/mobile/metro-now/common/utils/metro-line.utils.swift
✅ Files skipped from review due to trivial changes (2)
- apps/mobile/metro-now/common/const/api-const.swift
- apps/mobile/metro-now/metro-now.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
🧰 Additional context used
🪛 SwiftLint
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift
[Warning] 21-21: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
apps/mobile/metro-now/metro-now/ContentView.swift
[Warning] 9-9: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 10-10: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 11-11: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (18)
apps/mobile/metro-now/metro-now Watch App/pages/stop/stop-detail.view.swift (2)
6-9: Fix typo in property name.
The typo in the property name 'headsing' still needs to be addressed.
49-54: Optimize platform lookup performance.
The platform lookup operation is still performed on every view update.
apps/mobile/metro-now/metro-now Watch App/pages/platform/platform-detail.view.swift (2)
23-70: 🛠️ Refactor suggestion
Previous safety and state handling concerns remain valid.
The previous review comment about safety checks and state handling is still applicable. Additionally:
- The toolbar placement syntax can be improved
- The circle overlay dimensions should use a constant for consistency
- ToolbarItem(
- placement: .confirmationAction)
- {
+ ToolbarItem(placement: .confirmationAction) {
Text(metroLineName)
.overlay(
Circle()
- .size(width: 32, height: 32, anchor: .center)
+ .frame(width: 32, height: 32)
.stroke(.white.opacity(0.6), lineWidth: 3)
)72-86: 🛠️ Refactor suggestion
Improve network call implementation.
The previous review comment about error handling and debug prints remains valid. Additionally:
- Add loading state management
- Consider implementing request debouncing to prevent multiple simultaneous requests
+ @State private var isLoading = false
+
func getDepartures() {
+ guard !isLoading else { return }
+ isLoading = true
NetworkManager.shared
.getDepartures(stopIds: [], platformIds: [platformId]) { result in
DispatchQueue.main.async {
+ isLoading = false
switch result {
case let .success(departures):
self.departures = departures
case let .failure(error):
- print(error.localizedDescription)
+ // TODO: Implement proper error handling
}
}
}
}apps/mobile/metro-now/metro-now.xcodeproj/xcshareddata/xcschemes/metro-now.xcscheme (2)
1-82: LGTM for the remaining configuration
The build, profile, analyze, and archive configurations follow standard practices and are properly configured for debug/release as appropriate.
26-32: Consider adding test targets to the scheme
The TestAction section is configured but doesn't include any test targets. Consider adding relevant unit and UI test targets to ensure proper test coverage.
apps/mobile/metro-now/metro-now.xcodeproj/xcshareddata/xcschemes/metro-now Watch App.xcscheme (4)
1-8: LGTM: Scheme configuration is properly set up
The scheme is correctly configured with:
- Latest Xcode version (1610)
- Parallel builds enabled
- Automatic architecture selection
68-92: LGTM: Profile, Analyze, and Archive actions are properly configured
The configuration is correct with:
- Profile action set to Release build
- Archive action configured to reveal in organizer
- Analyze action set to Debug build
47-67: Verify location simulation requirements
Location simulation is enabled (allowLocationSimulation = "YES"). If the Watch App uses location services:
- Ensure proper location simulation data is provided for testing
- Verify location permission handling in the Watch App
#!/bin/bash
# Description: Check for location-related code in the Watch App
# Search for Core Location usage
echo "Checking Core Location usage..."
rg "import CoreLocation|CLLocation|requestWhenInUseAuthorization|requestAlwaysAuthorization" --type swift --glob 'apps/mobile/metro-now/**/*Watch*'
# Search for location permission keys in Info.plist
echo "Checking location permission keys..."
rg "NSLocationWhenInUseUsageDescription|NSLocationAlwaysUsageDescription" --type plist9-39: Verify the build dependencies between Watch App and main app
The build action entries look correct, including both the Watch App and the main app. However, ensure that the build order is appropriate since the Watch App might have dependencies on the main app's components.
✅ Verification successful
Based on the project file analysis, I can now provide a definitive response about the build dependencies between the Watch App and main app.
Build configuration is correct and optimized for Watch App independence
The build configuration in the Xcode project shows:
- The Watch App is properly configured as an independent target (
WKRunsIndependentlyOfCompanionApp = YES) - Both apps share common code through a synchronized file system group with explicit exceptions
- The main app has a proper "Embed Watch Content" build phase that includes the Watch App
- There's a clear target dependency where the main app depends on the Watch App, but not vice versa
The build order is appropriate since:
- The Watch App can build independently
- The main app includes the Watch App bundle during its build phase
- Shared code is properly managed through file system synchronization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential shared components between Watch App and main app
# Look for imports or shared utilities that might indicate dependencies
# Search for shared components or utilities
echo "Searching for shared components..."
fd -e swift -e h -e m --base-directory apps/mobile/metro-now/common
# Search for import statements in Watch App files that might reference main app
echo "Checking Watch App dependencies..."
rg "import" --type swift --glob 'apps/mobile/metro-now/**/*Watch*'
Length of output: 519
Script:
#!/bin/bash
# Let's try a different approach to check the Watch App dependencies
# First, let's find all Swift files in the Watch App directory
echo "Watch App Swift files:"
fd -e swift . apps/mobile/metro-now --glob '*Watch*'
# Check the project structure and potential shared targets
echo -e "\nProject structure:"
find apps/mobile/metro-now -name "*.xcodeproj" -type d -exec sh -c 'for f; do echo "Project: $f"; ls -R "$f"; done' sh {} +
# Look for shared frameworks or targets
echo -e "\nShared frameworks and targets:"
find apps/mobile/metro-now -name "*.xcscheme" -o -name "*.pbxproj" | xargs cat
Length of output: 24247
apps/mobile/metro-now/common/managers/network-manager.swift (2)
7-8: LGTM! Valid singleton initialization.
The addition of parentheses is a valid style choice and doesn't affect functionality.
113-113: Verify the impact of removing metroOnly filter.
The commented-out metroOnly parameter might significantly increase the number of departures returned. Please verify:
- The UI can handle the increased data volume
- Performance remains acceptable
- The change aligns with the app's requirements
Consider implementing server-side pagination if not already present to handle the potentially larger dataset efficiently.
apps/mobile/metro-now/metro-now.xcodeproj/project.pbxproj (6)
451-451: Critical: iOS 18 deployment target is not yet released
Also applies to: 485-485
449-450: Improved iPad support
Good addition of landscape orientations for iPad while keeping iPhone in portrait-only mode. This change aligns with iOS platform guidelines for different form factors.
Also applies to: 483-484
285-285: Enhanced build optimization
Good addition of dead code stripping to reduce the final binary size.
Also applies to: 347-347
387-387: Version inconsistency between Watch App and main app
Also applies to: 419-419, 456-456, 490-490
395-395: Review watchOS deployment target
The watchOS deployment target has been increased from 9 to 11, which might exclude users with older Apple Watch models.
#!/bin/bash
# Check if there's any documentation about the minimum watchOS version requirement
rg -i "watchos.*11|watch.*requirement" --type mdAlso applies to: 428-428
373-373: Version number update needs documentation
The project version has been updated to 30 across all targets. Such a significant version jump (from 1 to 30) should be documented.
Also applies to: 405-405, 438-438, 472-472
| func getColorByRouteName(_ routeNumber: Int?) -> Color { | ||
| guard let routeNumber else { | ||
| return FALLBACK_COLOR | ||
| } | ||
|
|
||
| // tram | ||
| if routeNumber < 100 { | ||
| if routeNumber >= 90 { | ||
| return .black | ||
| } | ||
|
|
||
| return .purple | ||
| } | ||
|
|
||
| // bus | ||
| if routeNumber >= 900 { | ||
| return .black | ||
| } | ||
|
|
||
| return .blue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve route number handling maintainability.
The current implementation uses magic numbers and nested conditions that could be more maintainable.
+/// Route number ranges for different transport types
+private enum RouteRanges {
+ static let tramMax = 100
+ static let specialTramMin = 90
+ static let busMin = 900
+}
+
+/// Returns the color for a given route number.
+/// - Parameter routeNumber: The route number
+/// - Returns: The corresponding transport type color
func getColorByRouteName(_ routeNumber: Int?) -> Color {
guard let routeNumber else {
return FALLBACK_COLOR
}
- // tram
- if routeNumber < 100 {
- if routeNumber >= 90 {
+ switch routeNumber {
+ case ..<RouteRanges.tramMax:
+ if routeNumber >= RouteRanges.specialTramMin {
return .black
}
-
return .purple
- }
-
- // bus
- if routeNumber >= 900 {
+ case RouteRanges.busMin...:
return .black
+ default:
+ return .blue
}
-
- return .blue
}📝 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.
| func getColorByRouteName(_ routeNumber: Int?) -> Color { | |
| guard let routeNumber else { | |
| return FALLBACK_COLOR | |
| } | |
| // tram | |
| if routeNumber < 100 { | |
| if routeNumber >= 90 { | |
| return .black | |
| } | |
| return .purple | |
| } | |
| // bus | |
| if routeNumber >= 900 { | |
| return .black | |
| } | |
| return .blue | |
| } | |
| /// Route number ranges for different transport types | |
| private enum RouteRanges { | |
| static let tramMax = 100 | |
| static let specialTramMin = 90 | |
| static let busMin = 900 | |
| } | |
| /// Returns the color for a given route number. | |
| /// - Parameter routeNumber: The route number | |
| /// - Returns: The corresponding transport type color | |
| func getColorByRouteName(_ routeNumber: Int?) -> Color { | |
| guard let routeNumber else { | |
| return FALLBACK_COLOR | |
| } | |
| switch routeNumber { | |
| case ..<RouteRanges.tramMax: | |
| if routeNumber >= RouteRanges.specialTramMin { | |
| return .black | |
| } | |
| return .purple | |
| case RouteRanges.busMin...: | |
| return .black | |
| default: | |
| return .blue | |
| } | |
| } |
| func getColorByRouteName(_ routeName: String?) -> Color { | ||
| guard let routeName else { | ||
| return FALLBACK_COLOR | ||
| } | ||
|
|
||
| if let routeNumber = Int(routeName) { | ||
| return getColorByRouteName(routeNumber) | ||
| } else if let metroLine = MetroLine(rawValue: routeName) { | ||
| return getColorByRouteName(metroLine) | ||
| } | ||
|
|
||
| // ferry | ||
| if routeName.hasPrefix("P") { | ||
| return Color.blue | ||
| } | ||
|
|
||
| // funicular | ||
| if routeName.hasPrefix("LD") { | ||
| return Color.blue | ||
| } | ||
|
|
||
| return FALLBACK_COLOR | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance string route name handling and documentation.
The function could benefit from:
- Constants for prefix values
- Better error handling
- Comprehensive documentation
+/// Route prefixes for special transport types
+private enum RoutePrefix {
+ static let ferry = "P"
+ static let funicular = "LD"
+}
+
+/// Returns the color for a given route name string.
+/// - Parameter routeName: The route identifier which can be:
+/// - A number (for trams and buses)
+/// - A metro line letter (A, B, C)
+/// - A ferry route (starts with P)
+/// - A funicular route (starts with LD)
+/// - Returns: The corresponding transport type color
func getColorByRouteName(_ routeName: String?) -> Color {
guard let routeName else {
return FALLBACK_COLOR
}
+ // Try parsing as number or metro line first
if let routeNumber = Int(routeName) {
return getColorByRouteName(routeNumber)
} else if let metroLine = MetroLine(rawValue: routeName) {
return getColorByRouteName(metroLine)
}
- // ferry
- if routeName.hasPrefix("P") {
+ // Handle special transport types
+ if routeName.hasPrefix(RoutePrefix.ferry) {
return Color.blue
}
- // funicular
- if routeName.hasPrefix("LD") {
+ if routeName.hasPrefix(RoutePrefix.funicular) {
return Color.blue
}
return FALLBACK_COLOR
}📝 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.
| func getColorByRouteName(_ routeName: String?) -> Color { | |
| guard let routeName else { | |
| return FALLBACK_COLOR | |
| } | |
| if let routeNumber = Int(routeName) { | |
| return getColorByRouteName(routeNumber) | |
| } else if let metroLine = MetroLine(rawValue: routeName) { | |
| return getColorByRouteName(metroLine) | |
| } | |
| // ferry | |
| if routeName.hasPrefix("P") { | |
| return Color.blue | |
| } | |
| // funicular | |
| if routeName.hasPrefix("LD") { | |
| return Color.blue | |
| } | |
| return FALLBACK_COLOR | |
| } | |
| /// Route prefixes for special transport types | |
| private enum RoutePrefix { | |
| static let ferry = "P" | |
| static let funicular = "LD" | |
| } | |
| /// Returns the color for a given route name string. | |
| /// - Parameter routeName: The route identifier which can be: | |
| /// - A number (for trams and buses) | |
| /// - A metro line letter (A, B, C) | |
| /// - A ferry route (starts with P) | |
| /// - A funicular route (starts with LD) | |
| /// - Returns: The corresponding transport type color | |
| func getColorByRouteName(_ routeName: String?) -> Color { | |
| guard let routeName else { | |
| return FALLBACK_COLOR | |
| } | |
| // Try parsing as number or metro line first | |
| if let routeNumber = Int(routeName) { | |
| return getColorByRouteName(routeNumber) | |
| } else if let metroLine = MetroLine(rawValue: routeName) { | |
| return getColorByRouteName(metroLine) | |
| } | |
| // Handle special transport types | |
| if routeName.hasPrefix(RoutePrefix.ferry) { | |
| return Color.blue | |
| } | |
| if routeName.hasPrefix(RoutePrefix.funicular) { | |
| return Color.blue | |
| } | |
| return FALLBACK_COLOR | |
| } |
| ForEach(departures, id: \.headsign) { departure in | ||
| ClosestStopPageListItemView( | ||
| routeLabel: departure.route, | ||
| routeLabelBackground: getColorByRouteName(departure.route), | ||
| headsign: departure.headsign, | ||
| departure: departure.departure.predicted, | ||
| nextHeadsign: nil, | ||
| nextDeparture: nil | ||
| ) | ||
| } |
There was a problem hiding this comment.
Use unique identifier for ForEach
Using headsign as an identifier could lead to issues if multiple departures have the same headsign. Consider using a combination of properties or a unique identifier.
- ForEach(departures, id: \.headsign) { departure in
+ ForEach(departures, id: \.self) { departure inNote: This assumes ApiDeparture conforms to Hashable. If it doesn't, you should either:
- Make ApiDeparture conform to Hashable
- Create a unique identifier by combining multiple properties
- Use a dedicated id property from your API response
Committable suggestion skipped: line range outside the PR's diff.
| let routeLabel: String = platform.routes[0].name | ||
| let routeLabelBackground: Color = getColorByRouteName(routeLabel) |
There was a problem hiding this comment.
Add array bounds check for platform routes
The code assumes that platform.routes always has at least one element. This could crash if a platform has no routes.
- let routeLabel: String = platform.routes[0].name
- let routeLabelBackground: Color = getColorByRouteName(routeLabel)
+ guard let firstRoute = platform.routes.first else {
+ return EmptyView() // or some error state view
+ }
+ let routeLabel: String = firstRoute.name
+ let routeLabelBackground: Color = getColorByRouteName(routeLabel)Committable suggestion skipped: line range outside the PR's diff.
| <LocationScenarioReference | ||
| identifier = "../../../../../../../../Library/Mobile Documents/com~apple~CloudDocs/Downloads/New file 1.gpx" | ||
| referenceType = "0"> | ||
| </LocationScenarioReference> |
There was a problem hiding this comment.
Remove hardcoded personal iCloud path
The location scenario references a personal iCloud path which could cause issues for other developers. Consider:
- Moving the GPX file to the project directory
- Using a relative path
- Adding the GPX file to version control
<LocationScenarioReference
- identifier = "../../../../../../../../Library/Mobile Documents/com~apple~CloudDocs/Downloads/New file 1.gpx"
+ identifier = "Locations/default.gpx"
referenceType = "0">📝 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.
| <LocationScenarioReference | |
| identifier = "../../../../../../../../Library/Mobile Documents/com~apple~CloudDocs/Downloads/New file 1.gpx" | |
| referenceType = "0"> | |
| </LocationScenarioReference> | |
| <LocationScenarioReference | |
| identifier = "Locations/default.gpx" | |
| referenceType = "0"> | |
| </LocationScenarioReference> |
| func getAllStops( | ||
| completed: @escaping (Result<[ApiStop], FetchErrorNew>) -> Void | ||
| ) { | ||
| guard let url = URL(string: "\(ENDPOINT)/stop/all") else { | ||
| completed(.failure(.invalidUrl)) | ||
| return | ||
| } | ||
|
|
||
| let task = URLSession.shared.dataTask( | ||
| with: URLRequest(url: url) | ||
| ) { | ||
| data, response, error in | ||
|
|
||
| if let _ = error { | ||
| completed(.failure(.general)) | ||
| return | ||
| } | ||
|
|
||
| guard let response = response as? HTTPURLResponse, response.statusCode == 200 else { | ||
| completed(.failure(.invalidResponse)) | ||
| return | ||
| } | ||
|
|
||
| guard let data else { | ||
| completed(.failure(.invalidData)) | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| let decoder = JSONDecoder() | ||
| let decodedResponse = try decoder.decode([ApiStop].self, from: data) | ||
| completed(.success(decodedResponse)) | ||
| return | ||
| } catch { | ||
| completed(.failure(.invalidData)) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| task.resume() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication.
The getAllStops method largely duplicates code from getMetroStops. Consider extracting the common networking and decoding logic into a reusable method.
Here's a suggested refactor:
+ private func fetchStops(endpoint: String, completed: @escaping (Result<[ApiStop], FetchErrorNew>) -> Void) {
+ guard let url = URL(string: endpoint) else {
+ completed(.failure(.invalidUrl))
+ return
+ }
+
+ let task = URLSession.shared.dataTask(with: URLRequest(url: url)) { data, response, error in
+ if let _ = error {
+ completed(.failure(.general))
+ return
+ }
+
+ guard let response = response as? HTTPURLResponse, response.statusCode == 200 else {
+ completed(.failure(.invalidResponse))
+ return
+ }
+
+ guard let data else {
+ completed(.failure(.invalidData))
+ return
+ }
+
+ do {
+ let decoder = JSONDecoder()
+ let decodedResponse = try decoder.decode([ApiStop].self, from: data)
+ completed(.success(decodedResponse))
+ } catch {
+ completed(.failure(.invalidData))
+ }
+ }
+ task.resume()
+ }
+
func getMetroStops(completed: @escaping (Result<[ApiStop], FetchErrorNew>) -> Void) {
- guard let url = URL(string: "\(ENDPOINT)/stop/all?metroOnly=true") else {
- completed(.failure(.invalidUrl))
- return
- }
- // ... rest of the implementation
+ fetchStops(endpoint: "\(ENDPOINT)/stop/all?metroOnly=true", completed: completed)
}
func getAllStops(completed: @escaping (Result<[ApiStop], FetchErrorNew>) -> Void) {
- guard let url = URL(string: "\(ENDPOINT)/stop/all") else {
- completed(.failure(.invalidUrl))
- return
- }
- // ... rest of the implementation
+ fetchStops(endpoint: "\(ENDPOINT)/stop/all", completed: completed)
}Additionally, consider these improvements:
- Add response caching using
URLCache - Configure request timeout using
URLRequest.timeoutInterval - Consider using async/await pattern for better readability
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
this will be added in separate PR
There was a problem hiding this comment.
@krystxf, understood. Would you like me to open a GitHub issue to track this refactoring suggestion?
| func getAllMetroStops() { | ||
| NetworkManager.shared.getMetroStops { result in | ||
| DispatchQueue.main.async { | ||
| self.location = location | ||
| switch result { | ||
| case let .success(stops): | ||
|
|
||
| metroStops = stops | ||
| print("Fetched metro stops") | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct ContentView: View { | ||
| @StateObject private var locationManager = LocationManager() | ||
| func getAllStops() { | ||
| NetworkManager.shared.getAllStops { result in | ||
| DispatchQueue.main.async { | ||
| switch result { | ||
| case let .success(stops): | ||
|
|
||
| var body: some View { | ||
| VStack { | ||
| if let location = locationManager.location { | ||
| Text("Latitude: \(location.coordinate.latitude)") | ||
| Text("Longitude: \(location.coordinate.longitude)") | ||
| } else { | ||
| Text("Fetching location...") | ||
| allStops = stops | ||
| print("Fetched all stops") | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
| .padding() | ||
| } |
There was a problem hiding this comment.
Enhance error handling in network calls.
The current implementation only prints errors to the console. Users should be notified of network issues, and the app should implement proper error handling and retry mechanisms.
Consider implementing a proper error handling strategy:
enum NetworkState {
case loading
case loaded
case error(Error)
}
@State private var networkState: NetworkState = .loading
func getAllMetroStops() {
networkState = .loading
NetworkManager.shared.getMetroStops { result in
DispatchQueue.main.async {
switch result {
case let .success(stops):
metroStops = stops
networkState = .loaded
case let .failure(error):
networkState = .error(error)
// Implement retry mechanism
DispatchQueue.main.asyncAfter(deadline: .now() + 5) {
getAllMetroStops()
}
}
}
}
}There was a problem hiding this comment.
I'll focus on fetching logic in separate PR
There was a problem hiding this comment.
@krystxf, understood. Would you like me to open a GitHub issue to track this enhancement?
| NetworkManager.shared | ||
| .getDepartures(stopIds: [closestStop.id], platformIds: []) { result in | ||
| DispatchQueue.main.async { | ||
| switch result { | ||
| case let .success(departures): | ||
|
|
||
| self.departures = departures | ||
| print("Fetched departures") | ||
|
|
||
| case let .failure(error): | ||
| print(error.localizedDescription) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make network requests cancellable to prevent memory leaks.
Network requests should be cancelled when the view disappears to prevent memory leaks and unnecessary network traffic.
private var cancellables = Set<AnyCancellable>()
func getStopDepartures() {
NetworkManager.shared
.getDepartures(stopIds: [closestStop.id], platformIds: [])
.sink(
receiveCompletion: { completion in
if case let .failure(error) = completion {
// Handle error
}
},
receiveValue: { departures in
self.departures = departures
}
)
.store(in: &cancellables)
}
// Add to view modifiers:
.onDisappear {
cancellables.forEach { $0.cancel() }
}
krystxf
force-pushed
the
feat/ios-app
branch
from
7960538 to
b0b3c1e
Compare
krystxf
force-pushed
the
feat/ios-app
branch
from
f458687 to
bffddc0
Compare
krystxf
force-pushed
the
feat/ios-app
branch
from
f27da1f to
6f4491a
Compare
krystxf
force-pushed
the
feat/ios-app
branch
from
6f4491a to
d69afb0
Compare
Summary by CodeRabbit
Release Notes
New Features
RouteNameIconViewfor displaying route labels with customizable backgrounds.PlatformDetailNextDepartureViewandPlatformDetailDeparturesListViewfor enhanced display of platform departure information.StopDeparturesViewfor listing metro platforms and their departures.ClosestStopPageListItemViewandClosestMetroStopSectionView.Bug Fixes
Chores