Migration/navigation 3 #100
Conversation
…avigation in AppNavigation
…ructure and enhance error handling in UI components
This commit introduces several key enhancements: - **Weather Alerts:** Adds a new `WeatherAlertCard` composable in `sunriseui` to display weather alert information with an expandable details section. This component is integrated into the `HomeScreen` via `WeatherAlertLayout` to show relevant alerts to the user. - **Network Model Update:** The `WeatherForecast` model in the `network` module has been updated to include `alerts` and a reusable `WeatherInfo` data class. This refactoring centralizes weather information structure across different parts of the forecast (current, daily, hourly). - **Network Exception Handling:** A new `NetworkException` class is introduced in `network.common` to provide more specific error codes and messages for network-related issues, improving error diagnosis and handling. - **UI Animation Refinement:** Animations in `HomeScreen` for displaying weather components have been adjusted for a smoother and more prioritized reveal of information, particularly weather alerts.
WalkthroughAdds authentication, payment, and feedback features; restructures network models and DI for token-based auth; integrates Razorpay checkout and premium state; refactors KMM/network DSL and versions; updates UI (login, premium, alerts), storage (AuthToken/TokenStorage), removes standalone payment module, and adds CI/linting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as MainActivity/Compose
participant VM as MainViewModel
participant Pay as Razorpay SDK
participant Net as PaymentRepository
Note over UI,VM: Launch checkout flow
User->>UI: Tap "Subscribe"
UI->>VM: startPayment()
VM->>Net: createOrder(request)
Net-->>VM: Result(CreateOrderResponse)
VM-->>UI: PaymentEvent.LaunchCheckout(keyId, orderId, amount, ...)
UI->>Pay: open(options)
alt Success
Pay-->>UI: onPaymentSuccess(paymentData)
UI->>VM: verifyPayment(orderId, paymentId, signature)
VM->>Net: verifyPayment(request)
Net-->>VM: Result(VerifyPaymentResponse)
VM-->>UI: paymentUiState = Success(expiryMillis)
else Failure
Pay-->>UI: onPaymentError(code, response)
UI->>VM: onPaymentFailed(message)
VM-->>UI: paymentUiState = Failure
end
sequenceDiagram
autonumber
participant App as App Layer
participant Http as HttpClient (Ktor)
participant TM as TokenManager
participant Srv as Backend API
Note over App,Http: Authenticated request
App->>Http: Request (defaultRequest)
Http->>TM: getValidToken()
TM-->>Http: Bearer token (if available)
Http->>Srv: Send request
alt 401 Unauthorized
Http-->>TM: handleUnauthorized()
TM->>Srv: refreshToken (via AuthRepository)
alt Refresh success
TM-->>Http: new token
Http->>Srv: Retry with token
Srv-->>Http: 200 OK
Http-->>App: Response
else Refresh failed
TM->>TM: forceLogout()
Http-->>App: Unauthorized error
Note over App: Observe AuthEventBus.Unauthorized
end
else 2xx/4xx/5xx (non-401)
Srv-->>Http: Response
Http-->>App: Response
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/src/main/res/values-iw/strings.xml (1)
37-38: Duplicate error message concern applies here as well.Same observation as in the Hindi localization file -
network_unavailable_txtduplicatesno_internet_txt. This pattern is consistent across localization files, suggesting it's part of the error handling design.app/src/main/res/values/strings.xml (1)
38-39: Same duplication pattern in the main strings file.The
network_unavailable_txtduplicatesno_internet_txthere as well, confirming this is a deliberate design choice across all localization files. The timeout message provides clear user feedback.
🧹 Nitpick comments (10)
app/src/main/res/values-hi/strings.xml (1)
37-38: Consider consolidating duplicate network error messages.The
network_unavailable_txtduplicates the existingno_internet_txtmessage. While this may provide semantic clarity in the code, it creates maintenance overhead. Consider whether these should be consolidated or if they serve distinct purposes in the error handling flow.The timeout message is well-crafted and provides clear user guidance.
app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt (1)
7-13: Good documentation but consider clarifying single alert behavior.The documentation is helpful but could be more explicit about only displaying the first alert when multiple alerts are present.
Consider enhancing the documentation:
* This composable is responsible for displaying weather alerts on the HomeScreen. - * It handles the case when there are no alerts by not rendering anything. + * It handles the case when there are no alerts by not rendering anything. + * When multiple alerts are present, only the first (most recent/important) alert is displayed.sunriseui/src/main/java/bose/ankush/sunriseui/components/WeatherAlertCard.kt (4)
94-96: Extract magic number to a named constantThe hard-coded value
100should be extracted to a named constant for better maintainability.Add a constant at the top of the file:
private const val SHORT_DESCRIPTION_LENGTH = 100Then update the code:
- val shortDescription = remember(description) { - if (description.length > 100) description.take(100) + "..." else description - } + val shortDescription = remember(description) { + if (description.length > SHORT_DESCRIPTION_LENGTH) description.take(SHORT_DESCRIPTION_LENGTH) + "..." else description + }
324-328: Document timestamp format expectationThe function assumes timestamps are in seconds (Unix time). Consider adding documentation to clarify this expectation.
/** * Formats a timestamp into a readable date and time string. + * @param timestamp Unix timestamp in seconds */ private fun formatTimestamp(timestamp: Long): String { val date = Date(timestamp * 1000) // Convert to milliseconds
179-186: Consider internationalization for accessibilityThe content description is hard-coded in English. For better accessibility across different locales, consider using string resources.
Instead of hard-coding "Weather Alert Icon", use a string resource:
Icon( imageVector = Icons.Filled.Warning, - contentDescription = "Weather Alert Icon", + contentDescription = stringResource(R.string.weather_alert_icon_description), tint = colors.primaryColor,
247-251: Remove unnecessary alpha animationThe button alpha animation always targets
1f, making it redundant. Either remove it or implement a proper fade-in effect.Remove the unnecessary animation:
- val buttonAlpha by animateFloatAsState( - targetValue = 1f, - animationSpec = tween(300, easing = FastOutSlowInEasing), - label = "Button Alpha" - ) - TextButton( onClick = onToggleExpanded, modifier = Modifier .align(Alignment.End) .padding(top = 4.dp) - .alpha(buttonAlpha)network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt (2)
19-25: Consider simplifying double nullable patternThe lists use double nullable pattern (
List<Alert?>?), which might be overly defensive. Consider whether you need both the list and its elements to be nullable.If the API can return null lists but never null elements within lists:
- val alerts: List<Alert?>?, + val alerts: List<Alert>?, - val daily: List<Daily?>?, + val daily: List<Daily>?, - val hourly: List<Hourly?>? + val hourly: List<Hourly>?If the API always returns lists (possibly empty) but elements can be null:
- val alerts: List<Alert?>?, + val alerts: List<Alert?> = emptyList(), - val daily: List<Daily?>?, + val daily: List<Daily?> = emptyList(), - val hourly: List<Hourly?>? + val hourly: List<Hourly?> = emptyList()
28-37: Inconsistent default value usage
WeatherInfoprovides default values for all properties, while other data classes use nullable types. Therainproperty inDailyhas a default null value while other nullable properties don't.For consistency, either:
- Remove defaults from
WeatherInfoto match other classes:data class WeatherInfo( @SerialName("description") - val description: String = "", + val description: String?, @SerialName("icon") - val icon: String = "", + val icon: String?, @SerialName("id") - val id: Int = 0, + val id: Int?, @SerialName("main") - val main: String = "" + val main: String? )
- Or remove the default from
rain:- val rain: Double? = null, + val rain: Double?,Also applies to: 89-89
network/src/commonMain/kotlin/bose/ankush/network/common/NetworkException.kt (2)
33-35: Consider improving regex pattern specificity.The current regex
(\\d{3})matches any 3-digit sequence, which could extract unintended numbers from error messages. Consider using a more specific pattern that looks for HTTP status codes in typical formats.-val errorCodeRegex = Regex("(\\d{3})") +val errorCodeRegex = Regex("(?:HTTP |status |code |error )?([45]\\d{2})")This pattern specifically looks for 4xx/5xx HTTP status codes with optional prefixes, reducing false positives.
33-35: Consider edge cases in error code extraction.The regex pattern
(\\d{3})will match any 3-digit number in the exception message, which might not always be an error code. Consider if this could lead to false positives.Example edge case: An exception message like "Failed to process 123 items" would extract "123" as an error code.
If this becomes problematic, consider a more specific pattern:
val errorCodeRegex = Regex("(?:error|status|code)[:\\s]*(\\d{3})", RegexOption.IGNORE_CASE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/src/main/res/drawable/zobo.pngis excluded by!**/*.png
📒 Files selected for processing (34)
app/build.gradle.kts(0 hunks)app/src/main/java/bose/ankush/weatherify/base/common/Constants.kt(0 hunks)app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt(0 hunks)app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/data/mapper/WeatherMapper.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/domain/model/WeatherForecast.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt(4 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt(6 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/AirQualityDetailsScreen.kt(0 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/HomeScreen.kt(7 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/state/ErrorComponent.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/presentation/navigation/AppNavigation.kt(0 hunks)app/src/main/java/bose/ankush/weatherify/presentation/navigation/Screen.kt(0 hunks)app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt(3 hunks)app/src/main/res/values-hi/strings.xml(1 hunks)app/src/main/res/values-iw/strings.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)buildSrc/src/main/java/ConfigData.kt(1 hunks)buildSrc/src/main/java/Versions.kt(2 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)language/build.gradle.kts(0 hunks)network/src/commonMain/kotlin/bose/ankush/network/api/KtorWeatherApiService.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/common/NetworkException.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/AirQuality.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/WeatherCondition.kt(0 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt(3 hunks)network/src/commonMain/kotlin/bose/ankush/network/utils/NetworkUtils.kt(4 hunks)payment/build.gradle.kts(0 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt(4 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherEntity.kt(1 hunks)sunriseui/src/main/java/bose/ankush/sunriseui/components/WeatherAlertCard.kt(1 hunks)
💤 Files with no reviewable changes (9)
- payment/build.gradle.kts
- app/src/main/java/bose/ankush/weatherify/presentation/navigation/Screen.kt
- app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt
- app/src/main/java/bose/ankush/weatherify/presentation/home/AirQualityDetailsScreen.kt
- app/src/main/java/bose/ankush/weatherify/base/common/Constants.kt
- network/src/commonMain/kotlin/bose/ankush/network/model/WeatherCondition.kt
- language/build.gradle.kts
- app/src/main/java/bose/ankush/weatherify/presentation/navigation/AppNavigation.kt
- app/build.gradle.kts
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt (1)
sunriseui/src/main/java/bose/ankush/sunriseui/components/WeatherAlertCard.kt (1)
WeatherAlertCard(53-167)
app/src/main/java/bose/ankush/weatherify/presentation/home/HomeScreen.kt (3)
app/src/main/java/bose/ankush/weatherify/presentation/home/state/ErrorComponent.kt (2)
ErrorBackgroundAnimation(32-39)ShowError(50-116)app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt (1)
WeatherAlertLayout(14-36)app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (1)
BriefAirQualityReportCardLayout(39-43)
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (1)
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (1)
errorResponseFromException(50-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qodana
🔇 Additional comments (91)
buildSrc/src/main/java/Versions.kt (3)
5-5: Kotlin 2.1.21 is a valid release
We’ve confirmed that Kotlin 2.1.21 was published on May 13, 2025 (it’s the latest in the 2.1.x line), and the current overall stable is 2.2.0 (June 23, 2025). You can proceed with 2.1.21 as specified.
56-57: Hilt versions compatibility confirmedHilt (
com.google.dagger:hilt-android:2.56.2) and Hilt Navigation Compose (androidx.hilt:hilt-navigation-compose:1.2.0) have been verified as compatible with each other and with the current Kotlin/Compose setup. No further changes are required.
8-8: Compose BOM version confirmed
The BOM version “2025.05.01” was released on May 20, 2025 and is a valid Compose BOM. The current stable BOM is “2025.07.00” (released July 16, 2025) – feel free to bump to that if you need the latest updates; otherwise, no change is required.buildSrc/src/main/java/ConfigData.kt (1)
3-5: Android API Level 36 Verified
- Android API level 36 corresponds to Android 16, released June 10, 2025 (source: Wikipedia).
- Skipping API 35 is safe as long as your dependencies support Android 16.
- Ensure SDK 36 is installed in your development and CI environments.
gradle/wrapper/gradle-wrapper.properties (1)
3-3: Gradle 8.14.3 meets AGP 8.11.1 requirementsVerified that Gradle 8.14.3 is the latest stable release (July 4, 2025) and that Android Gradle Plugin 8.11.1 requires a minimum Gradle version of 8.11.1. This update satisfies that requirement.
Consider scanning the Gradle and AGP release notes for any edge-case issues, but no blockers remain.network/src/commonMain/kotlin/bose/ankush/network/api/KtorWeatherApiService.kt (2)
32-32: LGTM! Endpoint standardization follows REST conventions.The endpoint path change from
/get-weatherto/weatheris consistent with REST API best practices and matches the air quality endpoint change.
22-22: All legacy endpoint references have been removedWe’ve confirmed that there are no remaining occurrences of
get-air-pollutionorget-weatheranywhere in the codebase. The new, standardized paths are now:
- GET
${baseUrl}/air-pollution- GET
${baseUrl}/weatherPlease coordinate with the backend team to ensure these endpoints are live and fully supported before merging.
app/src/main/java/bose/ankush/weatherify/data/mapper/WeatherMapper.kt (2)
5-6: Import statement updates reflect storage layer reorganization.The updated import statements align with the storage layer restructuring mentioned in the PR summary.
18-21: Excellent null safety enhancement!The Elvis operator usage with default empty strings is a solid defensive programming practice. This prevents null pointer exceptions and ensures consistent behavior when storage data is incomplete, improving the app's resilience to data inconsistencies.
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (3)
5-5: LGTM: Proper import of NetworkException for centralized error handling.Good addition of the NetworkException import to support the refactored error handling approach.
45-55: Well-designed exception mapping function with proper delegation.The
errorResponseFromExceptionfunction provides a clean interface for mapping any exception to user-friendly messages, with proper delegation to the specializederrorResponsefunction for NetworkException instances.
20-43: Excellent refactoring of error handling – all string resources verifiedThe refactored
errorResponsefunction provides several improvements:
- Replaces hardcoded error codes with named constants from
NetworkException- Adds comprehensive coverage for HTTP errors (400, 401, 403, 404, 500, 503) and network-specific errors
- Includes proper documentation
- Maintains backwards compatibility
All referenced string resources (
network_unavailable_txt,network_timeout_txt,city_error_txt,unauthorised_access_txt,server_error_txt,general_error_txt) are defined inapp/src/main/res/values/strings.xmland its localized variants.network/src/commonMain/kotlin/bose/ankush/network/repository/WeatherRepositoryImpl.kt (3)
65-66: LGTM: Proper initialization with nullable AirQuality fields.The initialization correctly uses the new AirQuality constructor with nullable fields, aligning with the restructured data models.
69-69: LGTM: Consistent fallback handling for nullable AirQuality.The map function properly provides a fallback AirQuality instance with nullable fields when the flow value is null.
127-127: LGTM: Simplified weather data assignment.Removing the lastUpdated timestamp manipulation aligns with the new nested data model structure where this field is likely handled differently.
app/src/main/java/bose/ankush/weatherify/domain/model/WeatherForecast.kt (1)
76-81: LGTM: Sensible default values improve object construction safety.Adding default empty string values for
description,icon, andmainproperties provides several benefits:
- Safer object instantiation without requiring all string fields
- Consistent with the broader data model restructuring
- Reduces null pointer risks compared to nullable alternatives
- Maintains required
idfield as non-nullableThis aligns well with the storage layer's nullable approach while providing safer defaults at the domain level.
app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt (2)
19-22: LGTM: Proper null safety with early return.The null and empty check with early return is a clean approach that prevents unnecessary processing and UI rendering.
24-25: LGTM: Safe handling of first alert extraction.The safe extraction of the first alert with null check is well implemented.
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (7)
41-42: Good architectural improvement by removing unused navigation dependency.The removal of the
NavControllerparameter simplifies this component and makes it purely focused on displaying air quality information. This aligns well with the removal of the AirQualityDetailsScreen and reduces unnecessary coupling.
46-46: Consistent parameter simplification maintains architectural coherence.The private
ShowUIfunction signature change is consistent with the public function change, maintaining a clean separation of concerns by focusing purely on data display.
41-42: Good architectural improvement - removing navigation concerns from display component.The removal of the
NavControllerparameter simplifies the component and follows the single responsibility principle. This makes the component more reusable and easier to test.
65-65: Minor UI adjustment approved.The padding change maintains consistent spacing while simplifying the layout.
5-14: Import organization looks clean.The import statements are well-organized and only include necessary dependencies for the layout functionality.
41-42: Function signature simplified correctly.The removal of the
NavControllerparameter aligns with the elimination of navigation functionality from this component, making it a pure display component.
65-65: Consistent padding applied.The padding modification maintains visual consistency with the overall design.
network/src/commonMain/kotlin/bose/ankush/network/utils/NetworkUtils.kt (13)
3-3: Appropriate import addition for enhanced error handling.The NetworkException import supports the new standardized error handling approach implemented in this utility function.
17-17: Clear documentation update improves API contract.The updated documentation correctly specifies that
NetworkExceptionis thrown, providing clear guidance for exception handling by API consumers.
26-27: Good addition of exception tracking for better error context.Tracking the last exception preserves valuable error context through retry attempts and enables better debugging information in the final exception.
32-41: Excellent improvement to exception handling consistency.The enhanced logic ensures all exceptions are converted to
NetworkExceptionwhile preserving original exception information. This creates a consistent error handling contract and leverages theNetworkException.fromExceptionfactory method appropriately.
50-55: Consistent fallback exception handling maintains error contract.The change from
IllegalStateExceptiontoNetworkExceptionwithUNKNOWN_ERRORcode maintains consistency with the function's error handling contract and preserves the original exception context.
3-3: Good documentation and import updates.The import of
NetworkExceptionand updated documentation properly reflect the enhanced exception handling approach.Also applies to: 17-17
26-26: Excellent exception handling enhancement.The addition of
lastExceptiontracking and the improved retry logic ensures:
- Consistent exception types (
NetworkException) are always thrown- Original exception context is preserved through the
causeparameter- Proper conversion of generic exceptions using
NetworkException.fromExceptionThis provides better error debugging and handling consistency across the application.
Also applies to: 32-41
50-55: Improved fallback exception handling.The fallback
NetworkExceptionwithUNKNOWN_ERRORcode and preservedlastExceptionas cause is much better than the previousIllegalStateException. This maintains consistency with the error handling framework.
3-3: Good addition of NetworkException import.The import aligns with the centralized error handling approach introduced in this PR.
17-17: Documentation updated correctly.The documentation now accurately reflects that
NetworkExceptionis thrown instead of a genericException.
26-26: Exception tracking variable added properly.The
lastExceptionvariable enables better error context preservation throughout retry attempts.
34-41: Excellent exception handling logic.The implementation ensures that on the final retry attempt, either the existing
NetworkExceptionis rethrown or a generic exception is converted toNetworkExceptionusing the factory method. This provides consistent exception types to callers.
50-55: Good defensive programming with the fallback exception.While this code should theoretically never execute due to the logic in the retry loop, providing a fallback
NetworkExceptionwith the last caught exception as cause is excellent defensive programming.network/src/commonMain/kotlin/bose/ankush/network/common/NetworkException.kt (9)
7-11: Well-designed exception class with proper inheritance and error categorization.The class properly extends
Exceptionwhile adding structured error categorization through theerrorCodeproperty. This enables both human-readable messages and programmatic error handling.
14-19: Comprehensive coverage of common HTTP error codes.The selected HTTP status codes cover the most important client and server error scenarios with clear, descriptive naming. The choice of codes (400, 401, 403, 404, 500, 503) addresses typical web API error cases effectively.
22-25: Well-structured network error codes with logical organization.The custom 1000+ range avoids HTTP code conflicts while covering essential network connectivity scenarios. The
UNKNOWN_ERRORfallback and logical code progression provide good extensibility.
31-42: Pragmatic factory method with reasonable error code extraction.The factory method appropriately handles exception conversion with proper preservation of original exception information. The regex approach for extracting 3-digit error codes is pragmatic, though it extracts the first match which may not always be the actual error code in complex messages.
7-11: Well-designed exception class structure.The class properly extends
Exceptionand includes anerrorCodefor categorizing network errors. The constructor parameters are appropriately defined with nullable cause.
14-25: Comprehensive error code constants.Good coverage of common HTTP status codes and network-specific errors. The separation between HTTP codes (400-500s) and network codes (1000s) provides clear categorization.
7-11: Well-designed exception class structure.The class properly extends
Exceptionand includes anerrorCodeproperty for structured error handling. The constructor parameters are appropriate with optionalcauseparameter.
13-25: Comprehensive error code constants.The error code constants cover common HTTP status codes and network-specific errors. The separation between HTTP codes (400-503) and network-specific codes (1000+) provides clear categorization.
31-42: Robust factory method implementation.The
fromExceptionmethod provides good error code extraction logic using regex for 3-digit codes, with appropriate fallback toUNKNOWN_ERROR. The method properly preserves the original exception as the cause.app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (9)
23-23: Appropriate import for edge-to-edge display functionality.The
WindowCompatimport supports the edge-to-edge display functionality implemented inonCreate, providing a modern Android UI experience.
102-127: Good formatting improvements enhance code readability.The reformatted permission handling composable improves readability while maintaining the same functional behavior. The cleaner structure makes the permission flow logic easier to follow.
137-150: Consistent formatting improvements maintain code style.The notification permission composable formatting is consistent with the location permission improvements, enhancing readability while preserving functionality.
23-23: Good addition for edge-to-edge display support.The
WindowCompatimport supports the edge-to-edge display functionality mentioned inonCreate(), providing a modern UI experience.
102-103: Code formatting improvements approved.The formatting adjustments improve code readability while preserving the existing permission request logic for location permissions.
Also applies to: 114-119
23-23: Good addition of WindowCompat import.The import supports the edge-to-edge display implementation mentioned in the onCreate method.
102-111: Clean formatting of permission launcher.The code formatting is improved and the permission handling logic remains intact and correct.
114-126: Consistent permission dialog handling.The permission dialog logic is well-structured and handles both fine and coarse location permissions appropriately with proper fallback.
137-150: Notification permission handling preserved.The notification permission request logic is maintained correctly with proper state updates and user feedback.
storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (14)
13-15: Clear import organization with effective type aliases.The type aliases with "Network" prefixes clearly distinguish between network and storage models, preventing naming conflicts and improving code clarity.
94-96: Proper handling of new nested API structure with null safety.The access to the nested
datafield correctly reflects the new API structure while implementing appropriate null handling for robust data processing.
101-125: Comprehensive and safe field mapping updates.The mapping correctly handles the naming convention changes (snake_case to camelCase), implements explicit type conversions, and includes robust null safety throughout. The
id ?: 0fallback and?.toLong()conversions are particularly well-handled.
185-186: Clear handling of alerts removal with appropriate documentation.The explicit null assignment for alerts with explanatory comment clearly communicates the intentional removal due to the new model structure, preventing confusion and potential issues.
94-96: Good adaptation to nested JSON structure.The new pattern accessing
weatherData.dataproperly reflects the updated API structure. The null safety check is appropriate for handling potential missing data.
100-125: Comprehensive mapping updates with improved null safety.The updates successfully handle:
- Field name changes from snake_case to camelCase (e.g.,
feels_like→feelsLike)- Explicit type conversions with null safety (
dt?.toLong())- Proper null handling with safe navigation operators
- Consistent mapping pattern across current, daily, and hourly data
These changes align well with the restructured network model and Kotlin naming conventions.
Also applies to: 126-163, 164-184
186-186: Intentional alerts removal documented.The explicit setting of
alerts = nullwith a clear comment explains that alerts are not available in the new model structure. This is better than silently dropping the field.
13-15: Import organization improved.The import aliases are clear and help distinguish between network and storage models.
94-96: Good handling of nested data structure.The code properly accesses the nested
datafield from the network model, preparing for null-safe access throughout the mapping.
100-101: Proper null safety implementation.The use of safe call operator (
?.) andletensures that the current weather data is only processed if the nested data is not null.
104-105: Correct field mapping updates.The field name changes from
dtto explicittoLong()conversion andfeels_liketofeelsLikereflect the updated network model structure properly.
117-117: Good default value handling.The use of
id ?: 0provides a sensible default when the weather condition ID is null, preventing potential issues downstream.
122-124: Consistent camelCase field mapping.The field name updates from
wind_gusttowindGustandwind_speedtowindSpeedare consistent with the camelCase convention adopted in the new network model.
185-186: Appropriate handling of removed alerts.Setting alerts to
nullis the correct approach since the new network model structure doesn't include alerts in the same format. This prevents mapping errors while maintaining the entity structure.app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (3)
9-9: Good addition of centralized error handling.The import of
errorResponseFromExceptionsupports the consistent error handling pattern implemented throughout this ViewModel.
93-99: Excellent improvement to error handling consistency.The refactored exception handler now uses the centralized
errorResponseFromExceptionfunction, which provides consistent error message mapping across the application. The type check ensures safe casting fromThrowabletoException.
169-169: Consistent error handling pattern applied throughout.All error handling locations now follow the same pattern:
- Type-safe casting from
ThrowabletoException- Using
errorResponseFromExceptionfor centralized error message mapping- Fallback to generic error message for non-Exception throwables
This provides better consistency and maintainability compared to scattered error handling logic.
Also applies to: 194-199, 206-206, 259-267, 289-289
network/src/commonMain/kotlin/bose/ankush/network/model/AirQuality.kt (3)
8-15: Well-structured nested data model with proper serialization.The top-level
AirQualityclass correctly reflects the new API structure withdata,message, andstatusfields. The use of@SerialNameannotations ensures proper JSON mapping, and nullable types handle potential missing data gracefully.
16-56: Good nested class design with comprehensive pollutant coverage.The nested structure is well-organized:
Datacontains the list of air quality measurementsItem9represents individual measurements with timestamp and componentsComponentsincludes all major pollutants (CO, NH3, NO, NO2, O3, PM10, PM2.5, SO2)Maincontains the overall AQI valueThe addition of
nh3andnopollutants enhances data completeness.
58-73: Excellent backward compatibility implementation.The helper properties provide a clean migration path by:
- Safely navigating the nested structure using null-safe operators
- Returning sensible defaults (0 for Int, 0.0 for Double) when data is unavailable
- Maintaining the same API surface for existing consumers
This approach allows gradual migration to the new nested structure without breaking existing code.
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (3)
67-67: Clean removal of unused navigation callback.The removal of
onAvatarNavActionparameter aligns with the broader cleanup of phone call permission features mentioned in the PR objectives.
70-70: Consistent cleanup of unused coroutine scope and navigation parameters.The removal of unused
rememberCoroutineScope()assignment and theonAvatarNavActionparameter fromScreenHeadercall maintains consistency with the function signature changes.Also applies to: 86-86
506-506: Simplified ScreenHeader function signature.The function signature is now cleaner with only the essential
modifierparameter, removing the avatar navigation functionality that was no longer needed.app/src/main/java/bose/ankush/weatherify/presentation/home/state/ErrorComponent.kt (6)
29-39: Simple and effective background component.The
ErrorBackgroundAnimationprovides a clean background using the theme's background color. The function name suggests animation, but the current implementation is static - consider if this is intentional or if animation will be added later.
41-56: Comprehensive documentation for the enhanced error component.The updated documentation clearly describes all parameters, including the new
isLoadingstate. This helps maintainability and usage understanding.
54-55: Excellent UX improvement with loading state management.The addition of the
isLoadingparameter with a default value offalseenables better user feedback during retry operations. The button'senabled = !isLoadingprevents multiple simultaneous retry attempts, which is a good UX practice.Also applies to: 96-96
65-65: Improved spacing and layout structure.The addition of horizontal padding and explicit
Spacercomponents creates better visual hierarchy and spacing consistency in the error display.Also applies to: 75-76, 86-87
71-71: Enhanced visual design with better error prominence.The styling improvements include:
- Smaller error icon (32dp vs 36dp) for better proportion
- Error color scheme for text and button for visual consistency
titleMediumtypography for better readabilityThese changes make the error state more visually prominent and consistent with Material Design guidelines.
Also applies to: 80-84, 91-97
98-113: Excellent loading indicator implementation in retry button.The conditional rendering shows either:
- A progress indicator with text when loading
- Just the text when not loading
The 16dp circular progress indicator with 2dp stroke width is appropriately sized, and the 8dp spacing provides good visual separation. This gives users clear feedback that their retry action is being processed.
app/src/main/java/bose/ankush/weatherify/presentation/home/HomeScreen.kt (6)
40-41: Good addition of new UI components.The imports for
WeatherAlertLayoutandErrorBackgroundAnimationsupport the enhanced error handling and new weather alerts feature.
94-126: Outstanding improvement to error handling UX.The enhanced
HandleScreenErrorfunction provides excellent user experience improvements:
- Loading state management: Tracks retry operations with visual feedback
- Automatic loading reset: 2-second delay provides appropriate feedback duration
- Visual background:
ErrorBackgroundAnimationcreates consistent error state appearance- Button state management: Prevents multiple simultaneous retries by setting loading state
This implementation significantly improves the user experience during error recovery.
139-139: Well-integrated weather alerts with proper animation sequencing.The addition of
alertsTransitionStateand its early timing (150ms delay) prioritizes showing weather alerts to users quickly, which is appropriate for potentially urgent information. The animation state management is consistent with other UI elements.Also applies to: 148-148, 157-159
211-231: Excellent implementation of weather alerts UI.The weather alerts section:
- Uses proper animation with consistent timing and effects
- Includes optional
onReadMoreClickcallback for future extensibility- Has a placeholder for analytics or navigation
- Follows the same animation patterns as other sections
The conditional rendering based on
weatherReports?.alertsensures the section only appears when alerts are available.
245-245: Clean simplification of air quality component.The removal of the
NavControllerparameter fromBriefAirQualityReportCardLayoutaligns with the broader removal of air quality details navigation mentioned in the PR objectives. This simplifies the component's interface and responsibilities.
160-167: Optimized animation timing for better user experience.The reduced delays (150ms instead of longer intervals) create a more responsive feel while still maintaining the staggered animation effect. This provides a good balance between visual appeal and perceived performance.
| WeatherAlertCard( | ||
| title = firstAlert.event, | ||
| description = firstAlert.description, | ||
| startTime = firstAlert.start?.toLong(), | ||
| endTime = firstAlert.end?.toLong(), | ||
| source = firstAlert.sender_name, | ||
| onReadMoreClick = onReadMoreClick | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify property name consistency across data layers.
The implementation correctly handles nullable fields and type conversion, but the property name sender_name should be verified for consistency with the data model structure changes mentioned in the AI summary.
Please verify that the property names match across all data layers:
🏁 Script executed:
#!/bin/bash
# Check for sender_name vs senderName consistency in alert-related code
rg -A 3 -B 3 "(sender_name|senderName)" --type kotlinLength of output: 3741
Unify sender_name to senderName across all layers
Right now the network model uses
@SerialName("sender_name") val senderName: String?but the storage entity, domain model, mapper and UI all declare
val sender_name: String?This inconsistency forces snake_case in Kotlin and makes the mapping harder to follow. Let’s switch everything to idiomatic camelCase and use annotations to preserve the JSON/DB field name.
Files to update:
- storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherEntity.kt
- val sender_name: String?, + @ColumnInfo(name = "sender_name") + val senderName: String?,
- app/src/main/java/bose/ankush/weatherify/domain/model/WeatherForecast.kt
- val sender_name: String?, + val senderName: String?,
- app/src/main/java/bose/ankush/weatherify/data/mapper/WeatherMapper.kt
- sender_name = it.sender_name, + senderName = it.senderName,
- app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt
- source = firstAlert.sender_name, + source = firstAlert.senderName,
After these changes, the network, storage, domain, mapper, and UI layers all refer to the property as senderName in Kotlin, with annotations handling the JSON/DB sender_name mapping.
🤖 Prompt for AI Agents
In
app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt
and related files around lines 28 to 35, unify the property name from snake_case
sender_name to camelCase senderName across all layers including storage, domain,
mapper, and UI. Update the property declarations to use senderName in Kotlin
code and add appropriate annotations like @SerialName("sender_name") or database
column annotations to preserve the original JSON/DB field name. This will ensure
consistent idiomatic Kotlin naming while maintaining correct serialization and
database mapping.
| @Serializable | ||
| data class Alert( | ||
| val description: String?, | ||
| val end: Int?, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify timestamp type change impact
The dt, start, end, sunrise, and sunset fields changed from Long? to Int?. Ensure all timestamp calculations throughout the codebase handle this change correctly.
Also applies to: 53-53, 83-83, 93-94, 129-129
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining Long type usage with these timestamp fields
# Search for potential Long type usage with these timestamp fields
rg -A 3 -B 3 '(dt|start|end|sunrise|sunset).*Long' --type kotlin
# Search for timestamp calculations that might assume Long type
rg -A 5 '(\.dt|\.start|\.end|\.sunrise|\.sunset).*\*.*1000' --type kotlinLength of output: 14626
Inconsistent timestamp types across modules
The network model now uses Int? for dt, start, end, sunrise, and sunset, but I’m still seeing Long? usages for these fields in multiple layers:
- sunriseui/src/main/java/bose/ankush/sunriseui/components/WeatherAlertCard.kt
• startTime: Long?
• endTime: Long? - sunriseui/src/main/java/bose/ankush/sunriseui/components/SunriseSunsetAnimation.kt
• sunriseTimestamp: Long?
• sunsetTimestamp: Long?
• currentTimestamp: Long - storage/src/commonMain/kotlin/bose/ankush/storage/api/WeatherStorage.kt
• getLastWeatherUpdateTime(): Long - storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherEntity.kt
• Current.dt: Long?
• Daily.dt: Long?
• Hourly.dt: Long? - storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt
• mappingdt = it.dt?.toLong()in Current, Daily, Hourly - app/src/main/java/bose/ankush/weatherify/presentation/home/HomeScreen.kt
• sunriseTimestamp = currentWeather.sunrise?.toLong()
• sunsetTimestamp = currentWeather.sunset?.toLong() - app/src/main/java/bose/ankush/weatherify/presentation/home/component/WeatherAlertLayout.kt
• startTime = firstAlert.start?.toLong()
• endTime = firstAlert.end?.toLong() - app/src/main/java/bose/ankush/weatherify/domain/model/WeatherForecast.kt
• Current.dt: Long?
• Daily.dt: Long?
• Hourly.dt: Long?
No stray *1000 multiplications were detected, so unit conversions look safe—but the type mismatch remains.
Please align timestamp types across modules: either revert the network fields to Long? or update domain, storage, and UI layers to Int? (and remove the unnecessary .toLong() calls).
🤖 Prompt for AI Agents
In network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt at
line 42, the timestamp fields use Int? while other modules use Long?. To fix the
inconsistency, choose one type for timestamps across all modules. Either revert
the network model fields (dt, start, end, sunrise, sunset) back to Long? or
update all domain, storage, and UI layers to use Int? and remove all .toLong()
conversions. Ensure all related fields and mappings are updated accordingly to
maintain type consistency.
| data class Weather( | ||
| val description: String, | ||
| val icon: String, | ||
| val description: String? = null, | ||
| val icon: String? = null, | ||
| val id: Int, | ||
| val main: String | ||
| val main: String? = null | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider database migration for nullable field changes.
Making previously non-nullable fields (description, icon, main) nullable is a breaking change for existing database schemas. This could cause issues if existing data doesn't have proper null handling or if Room migrations aren't properly defined.
Please verify that proper Room database migrations are in place to handle this schema change:
🏁 Script executed:
#!/bin/bash
# Search for Room database migration definitions
rg -A 10 -B 5 "Migration|@Database.*version.*=" --type kotlinLength of output: 1669
Ensure Proper Migration Strategy for Nullable Field Updates
A search in StorageModule.kt shows you’re using .fallbackToDestructiveMigration(), and no custom Migration objects were found. With your new nullable columns (description, icon, main), this means any schema version bump will drop and recreate the database—losing all existing weather data.
Please replace or supplement the destructive fallback with explicit Room migrations to preserve data:
• File:
storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt
– Current builder uses .fallbackToDestructiveMigration()
• No Migration definitions detected in the codebase
Action items:
- Define a
Migration(oldVersion, newVersion)that alters theWeatherEntitytable to allow nullable fields without data loss. - Register these migrations with
.addMigrations(...)on yourRoom.databaseBuilderand remove (or narrow) the destructive fallback.
🤖 Prompt for AI Agents
In storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherEntity.kt
around lines 78 to 83, the new nullable fields in the Weather data class require
a proper Room migration to avoid data loss. To fix this, define a Migration
object in storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt
that updates the WeatherEntity table schema to support nullable columns without
dropping data. Then, register this Migration with the Room.databaseBuilder using
.addMigrations(...) and remove or restrict the use of
.fallbackToDestructiveMigration() to prevent destructive database resets on
version upgrades.
…odules
This commit introduces a comprehensive authentication system and several enhancements across the application:
- **Authentication System:**
- Adds `AuthApiService`, `AuthRepository`, `TokenStorage`, and `TokenManager` for handling user login, registration, token refresh, and secure token storage.
- Implements Ktor-based `AuthApiService` and Room-based `TokenStorage`.
- Integrates authentication into `MainActivity` and `MainViewModel`, showing a new `LoginScreen` if the user is not authenticated.
- `LoginScreen` (`sunriseui`) provides UI for email/password input, login/register toggle, and links to terms/privacy policy.
- Device information (model, OS, app version, IP) is now collected and sent during registration.
- **Network Module:**
- `HttpClient` is now configured with an authentication interceptor (`configureAuth`) to automatically add JWT tokens to requests and handle 401 Unauthorized responses by attempting token refresh.
- `createWeatherRepository` and `createAuthRepository` factory functions are updated to support authenticated API calls.
- `AuthResponse`, `LoginRequest`, `RegisterRequest`, `RefreshTokenRequest` models are added.
- **UI Module (`sunriseui`):**
- Introduces `LoginScreen` composable with form validation, animated header, and loading states.
- Adds `GlassmorphicSnackbar` for displaying error messages with a glassmorphic effect.
- Removes Lottie dependency from `sunriseui` and `app` modules.
- **Storage Module:**
- `WeatherDatabase` is updated to version 3 to include `AuthToken` entity and `AuthTokenDao` for storing authentication tokens.
- `TokenStorageImpl` provides the Room-based implementation for `TokenStorage`.
- **App Module:**
- `build.gradle.kts` updated to enable `kotlin.RequiresOptIn` and `androidx.compose.animation.ExperimentalAnimationApi`.
- `MainActivity` now observes login state and navigates to `LoginScreen` or `AppNavigation` accordingly. Auth errors are displayed using `GlassmorphicSnackbar`.
- `MainViewModel` includes new logic for login, registration, logout, and managing authentication state (`AuthState`).
- New extension functions in `Extension.kt` for opening URLs, and retrieving device/app information.
- **Other Changes:**
- `WeatherForecast.Data.alerts` is now nullable with a default value.
- Minor formatting update in `AndroidHttpClient.kt`.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt (1)
52-54: Timestamp type changed to Int?; align types across modulesThis mirrors an earlier review: network switched dt/start/end/sunrise/sunset to Int?, while domain/storage/UI still use Long? in multiple places. Aligning these is required to avoid mismatches and conversions sprinkled around the code.
Quick checks:
#!/bin/bash # Fields defined as Long? downstream rg -nP -C2 '\b(dt|start|end|sunrise|sunset)\s*:\s*Long\??' --type kotlin # Conversions suggesting mismatched types rg -nP -C2 '\.(dt|start|end|sunrise|sunset)\b[^\n]{0,80}\btoLong\(' --type kotlinRecommendation:
- Either revert these fields in network models to Long? to match downstream, or
- Propagate Int? to domain/storage/UI and remove ad-hoc .toLong() calls.
Also applies to: 61-63, 83-83, 93-95, 129-129
🧹 Nitpick comments (50)
app/src/test/java/bose/ankush/weatherify/base/DateTimeUtilsTest.kt (7)
26-29: Limit object mocking to only the test that needs it (avoid global object mocking in setup).Mocking the DateTimeUtils object at setup can intercept all calls to its functions across tests. Unless explicitly stubbed, this can lead to unexpected behavior or “no answer found” errors. Prefer to mock only in the specific test that stubs behavior, or use a spy if you want real calls by default.
Apply this diff to move the mocking into the relevant test:
@@ fun setup() { MockKAnnotations.init(this) - mockkObject(DateTimeUtils) originalTimeZone = TimeZone.getDefault() TimeZone.setDefault(TimeZone.getTimeZone("UTC")) } @@ fun `verify that getDayWiseDifferenceFromToday returns day difference successfully`() { + mockkObject(DateTimeUtils) every { DateTimeUtils.getDayWiseDifferenceFromToday(now.toInt()) } returns 0 val numberOfDays = DateTimeUtils.getDayWiseDifferenceFromToday(now.toInt()) assertThat(numberOfDays).isEqualTo(0) }Also applies to: 45-47
18-18: Stabilize day-name assertions by pinning Locale (avoid flaky results on non-English locales).The "Thursday" expectation depends on Locale. Without pinning, tests can fail for non-English environments. Pin Locale to a known value in setup and restore it in teardown.
Apply this diff:
@@ - private lateinit var originalTimeZone: TimeZone + private lateinit var originalTimeZone: TimeZone + private lateinit var originalLocale: java.util.Locale @@ fun setup() { MockKAnnotations.init(this) originalTimeZone = TimeZone.getDefault() TimeZone.setDefault(TimeZone.getTimeZone("UTC")) + originalLocale = java.util.Locale.getDefault() + java.util.Locale.setDefault(java.util.Locale.US) @@ fun teardown() { TimeZone.setDefault(originalTimeZone) + java.util.Locale.setDefault(originalLocale) unmockkAll() }If you prefer separate imports, add:
import java.util.LocaleAlso applies to: 25-29, 36-36
41-48: This test only asserts the stub, not behavior (low signal).You’re stubbing
getDayWiseDifferenceFromTodayand then asserting the stubbed value. This doesn’t validate the implementation. Prefer verifying real behavior.Options:
- Refactor the production API to accept a “current time”/Clock/TimeProvider so the test can be deterministic without mocking the function under test.
- Or compute the expected difference deterministically (using the fixed epoch and UTC) and assert against the real function.
I can help sketch a lightweight TimeProvider and refactor if you want.
51-51: Fix typo: “Calender” → “Calendar”.Minor spelling issue in doc and test name string.
- * Verify that getTodayDateInCalenderFormat returns the current year + * Verify that getTodayDateInCalendarFormat returns the current year @@ - fun `verify that getTodayDateInCalenderFormat returns correct year number`() { + fun `verify that getTodayDateInCalendarFormat returns correct year number`() {Also applies to: 54-54
55-58: Strengthen the assertion to cover month/day as well (optional).Asserting only the year is permissive. Consider asserting month/day for better coverage:
- val todaysYear = DateTimeUtils.getTodayDateInCalenderFormat().get(Calendar.YEAR) - val expectedYear = Calendar.getInstance().get(Calendar.YEAR) - assertThat(todaysYear).isEqualTo(expectedYear) + val todayCal = DateTimeUtils.getTodayDateInCalenderFormat() + val expectedCal = Calendar.getInstance() + assertThat(todayCal.get(Calendar.YEAR)).isEqualTo(expectedCal.get(Calendar.YEAR)) + assertThat(todayCal.get(Calendar.MONTH)).isEqualTo(expectedCal.get(Calendar.MONTH)) + assertThat(todayCal.get(Calendar.DAY_OF_MONTH)).isEqualTo(expectedCal.get(Calendar.DAY_OF_MONTH))
27-29: Global TimeZone mutation can cause flaky tests when run in parallel.Setting the default TimeZone is process-wide. If your test runner parallelizes tests, this can bleed across tests. Ensure tests run sequentially or, better, accept a ZoneId/TimeZone in the production code to avoid global mutation.
If sequential execution is guaranteed in your CI, consider documenting that assumption here.
Also applies to: 36-36
45-46: Prefer Long for epoch seconds to avoid 2038 overflow
The current API only accepts an Int, and the test is forced to truncate a Long (now.toInt()), which risks precision loss and the 2038 overflow. To preserve full epoch‐second precision, consider adding a Long overload and updating call sites:
- In DateTimeUtils.kt, add:
fun getDayWiseDifferenceFromToday(epochSeconds: Long): Int = getDayWiseDifferenceFromToday(epochSeconds.toInt())- Update the test (and any callers) to pass the Long directly:
- every { DateTimeUtils.getDayWiseDifferenceFromToday(now.toInt()) } returns 0
- val numberOfDays = DateTimeUtils.getDayWiseDifferenceFromToday(now.toInt())
- every { DateTimeUtils.getDayWiseDifferenceFromToday(now) } returns 0
- val numberOfDays = DateTimeUtils.getDayWiseDifferenceFromToday(now)
This avoids unintentional truncation today and the 2038 boundary issue tomorrow. </blockquote></details> <details> <summary>app/build.gradle.kts (2)</summary><blockquote> `63-66`: **Prefer -opt-in over -Xopt-in (Kotlin 1.8+)** The non-X form is the stable flag; functionality is identical. Optional cleanup: Apply: ```diff - freeCompilerArgs = freeCompilerArgs + listOf( - "-Xopt-in=kotlin.RequiresOptIn", - "-Xopt-in=androidx.compose.animation.ExperimentalAnimationApi" - ) + freeCompilerArgs = freeCompilerArgs + listOf( + "-opt-in=kotlin.RequiresOptIn", + "-opt-in=androidx.compose.animation.ExperimentalAnimationApi" + )Alternatively, consider the new Kotlin DSL:
kotlin { compilerOptions { freeCompilerArgs.addAll( "-opt-in=kotlin.RequiresOptIn", "-opt-in=androidx.compose.animation.ExperimentalAnimationApi" ) } }
76-80: Disabling StrongSkipping has perf implications—document intentIf this is a temporary workaround for a known Compose issue, add a brief comment with a link to the bug/commit to avoid accidental re-enabling or future confusion.
Apply:
composeCompiler { + // NOTE: StrongSkipping disabled intentionally due to <link to issue/PR>. + // Revisit after upgrading Compose/Kotlin when the underlying bug is fixed. featureFlags = setOf( ComposeFeatureFlag.StrongSkipping.disabled() ) }Confirm this is intentional across all build types; disabling skipping can increase recomposition cost.
build.gradle.kts (2)
35-51: Deep clean task: safe for repo-local cleanup; consider warning usersIt deletes each module’s build dir and the repo’s .gradle directory (not ~/.gradle) which is generally safe but can cause longer next builds. Add a note in the task description or README for contributors.
Apply:
tasks.register("deepClean") { - description = "Cleans every module and removes all build artefacts in this repo." + description = "Cleans every module and removes all build artefacts in this repo (will cause full re-download/rebuild next run)." group = "build setup"
80-102: Detekt jvmTarget 21 may mismatch modules targeting 17Your Android modules target Java/Kotlin 17. Align Detekt’s jvmTarget to 17 to avoid false positives and classpath analysis issues.
Apply:
- tasks.withType<io.gitlab.arturbosch.detekt.Detekt>().configureEach { - jvmTarget = "21" + tasks.withType<io.gitlab.arturbosch.detekt.Detekt>().configureEach { + jvmTarget = "17" reports { xml.required.set(false) txt.required.set(false) sarif.required.set(false) md.required.set(false) html.required.set(true) } }network/src/commonMain/kotlin/bose/ankush/network/auth/storage/TokenStorage.kt (1)
22-27: Prefer exposing a token Flow over a boolean FlowA
Flow<Boolean>loses information and often forces extra reads. ExposingFlow<String?>(orStateFlow<String?>) lets consumers react to both presence and value changes, and derivehasTokenas needed.For example:
- /** - * Check if a token exists - * @return Flow of Boolean indicating if a token exists - */ - fun hasToken(): Flow<Boolean> + /** + * Observe the stored token (null when cleared) + * Consumers can map to availability using `.map { it != null }` + */ + fun tokenFlow(): Flow<String?>If you prefer to keep both, add
tokenFlow()and keephasToken()as a convenience wrapper.sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt (2)
49-55: Reset auto-dismiss when message changes while visibleIf
showis called with a new message while already visible, the timer does not reset because the effect is keyed only toisVisible. Key it tomessageas well.- LaunchedEffect(isVisible) { + LaunchedEffect(isVisible, message) { if (isVisible) { delay(durationMillis) onDismiss() } }
110-131: Avoid Pair; return a typed state holder for clarity and extensibilityReturning
Pair<(String) -> Unit, @Composable () -> Unit>obscures intent at call sites. A small wrapper improves readability and future-proofs API.Example:
@Stable class GlassmorphicSnackbarState internal constructor( private val setMessage: (String) -> Unit, val Content: @Composable () -> Unit ) { fun show(message: String) = setMessage(message) } @Composable fun rememberGlassmorphicSnackbarState( modifier: Modifier = Modifier ): GlassmorphicSnackbarState { var isVisible by remember { mutableStateOf(false) } var message by remember { mutableStateOf("") } val show: (String) -> Unit = { message = it isVisible = true } val content: @Composable () -> Unit = { GlassmorphicSnackbar( message = message, isVisible = isVisible, onDismiss = { isVisible = false }, modifier = modifier ) } return remember { GlassmorphicSnackbarState(show, content) } }sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt (4)
69-75: Persist UI state across recompositions/rotations with rememberSaveableEmail, password, visibility, and mode should survive configuration changes. Use
rememberSaveable.- var email by remember { mutableStateOf("") } - var password by remember { mutableStateOf("") } - var isPasswordVisible by remember { mutableStateOf(false) } - var isLoginMode by remember { mutableStateOf(true) } + var email by rememberSaveable { mutableStateOf("") } + var password by rememberSaveable { mutableStateOf("") } + var isPasswordVisible by rememberSaveable { mutableStateOf(false) } + var isLoginMode by rememberSaveable { mutableStateOf(true) }Add import:
import androidx.compose.runtime.saveable.rememberSaveableAlso applies to: 77-79
84-86: Trim email before validating formatAvoid false negatives due to leading/trailing spaces.
- val isEmailValid = { email: String -> - android.util.Patterns.EMAIL_ADDRESS.matcher(email).matches() - } + val isEmailValid = { email: String -> + android.util.Patterns.EMAIL_ADDRESS.matcher(email.trim()).matches() + }
124-132: Trim email before submitting; avoid mutating passwordsUsers often paste emails with whitespace; trim before submitting while keeping password verbatim.
- val handleSubmit = { + val handleSubmit = { if (!isLoading && validateInputs()) { + val trimmedEmail = email.trim() if (isLoginMode) { - onLoginClick(email, password) + onLoginClick(trimmedEmail, password) } else { - onRegisterClick(email, password) + onRegisterClick(trimmedEmail, password) } } }
168-175: Externalize user-facing strings to resources for i18n/accessibilityHardcoded text makes localization and theming harder. Prefer
stringResource(...)and contentDescription where applicable.Example:
import androidx.compose.ui.res.stringResource Text(text = stringResource(R.string.welcome_back)) OutlinedTextField(label = { Text(stringResource(R.string.email_address)) }, ...)If the Sunrise UI module is meant to be reusable, consider accepting string parameters or a
Stringsinterface to avoid hard dependency on app resources.Also applies to: 202-214, 231-237, 259-283, 299-325, 335-344, 347-375, 377-409
app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (2)
37-40: Harden openUrlInBrowser against missing handlers and non-Activity contextsStarting the intent can crash if no browser handles it, or when called from a non-Activity context. Add CATEGORY_BROWSABLE, conditionally add FLAG_ACTIVITY_NEW_TASK, and guard with try/catch.
- fun Context.openUrlInBrowser(url: String) { - val intent = Intent(Intent.ACTION_VIEW, url.toUri()) - startActivity(intent) - } + fun Context.openUrlInBrowser(url: String) { + val intent = Intent(Intent.ACTION_VIEW, url.toUri()).apply { + addCategory(Intent.CATEGORY_BROWSABLE) + if (this@openUrlInBrowser !is android.app.Activity) { + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + } + } + try { + startActivity(intent) + } catch (_: android.content.ActivityNotFoundException) { + // No browser available. Optionally log or show a fallback UI. + } + }
135-151: Prefer IPv4 and skip link-local/loopback for a more useful IPThis routine can return IPv6 or non-routable addresses. If you want a typical local IPv4, filter accordingly.
- if (!inetAddress.isLoopbackAddress && !inetAddress.isLinkLocalAddress) { + if (!inetAddress.isLoopbackAddress && + !inetAddress.isLinkLocalAddress && + inetAddress is java.net.Inet4Address + ) { return inetAddress.hostAddress }If you also want a public IP, you'd need a server-side echo or an external API; local interface scanning won’t provide it.
network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt (2)
9-14: Backticks around property namedataare unnecessaryKotlin allows a property named data without escaping. Removing backticks simplifies readability without changing behavior.
Apply this diff:
- @SerialName("data") - val `data`: Data?, + @SerialName("data") + val data: Data?,
19-19: Prefer safe defaults for collections to reduce null-checksLists are nullable (and can decode to null). Defaulting to emptyList() improves call-site ergonomics and avoids repetitive null handling when fields are absent.
Apply this diff:
- val alerts: List<Alert?>? = null, + val alerts: List<Alert?>? = emptyList(), ... - val daily: List<Daily?>?, + val daily: List<Daily?>? = emptyList(), - val hourly: List<Hourly?>? + val hourly: List<Hourly?>? = emptyList() ... - val weather: List<WeatherInfo?>?, + val weather: List<WeatherInfo?>? = emptyList(), ... - val weather: List<WeatherInfo?>?, + val weather: List<WeatherInfo?>? = emptyList(), ... - val weather: List<WeatherInfo?>? + val weather: List<WeatherInfo?>? = emptyList()Note: This does not handle explicit nulls from the server; if the API returns null explicitly, these will still be null. To guarantee non-null lists, make the properties non-nullable.
Also applies to: 23-25, 69-69, 101-101, 137-137
storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherDatabase.kt (1)
7-9: DB version bump with fallbackToDestructiveMigration wipes data; confirm that’s acceptableIncluding AuthToken and bumping to version 3 is fine, but combined with fallbackToDestructiveMigration (see StorageModule) this will delete cached weather and any stored token on upgrade. If this is not intended, add a proper Migration(2, 3) instead.
storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt (1)
46-54: Destructive migrations are enabled; evaluate impact on auth and cachefallbackToDestructiveMigration() will clear the DB (including auth token) on schema change. If preserving the token matters, consider adding concrete migrations instead.
storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthToken.kt (1)
9-15: Encrypt tokens at restThe DAO already uses
OnConflictStrategy.REPLACE, but raw tokens are still stored in plaintext. To reduce risk if the database is compromised, migrate token storage to an encrypted solution:
- AuthTokenDao.kt: confirms
@Insert(onConflict = OnConflictStrategy.REPLACE)- storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthToken.kt: persists
token: Stringwithout encryptionConsider using Android Keystore–backed EncryptedSharedPreferences or enabling SQLCipher/Room encryption for
auth_tokens.app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt (1)
36-47: Potential duplication: WeatherRepository factory builds its own AuthRepository while DI also provides onecreateWeatherRepository internally creates an AuthRepository/TokenManager, but this module additionally provides an AuthRepository. You’ll end up with two distinct instances. Consider passing the provided AuthRepository into the WeatherRepository factory (or exposing a variant that accepts TokenManager/AuthRepository) to keep a single source of truth.
Example API shape (in network module, outside this file):
// network/src/commonMain/.../NetworkModule.kt fun createWeatherRepository( networkConnectivity: NetworkConnectivity, authRepository: AuthRepository, // pass DI-provided instance tokenStorage: TokenStorage, baseUrl: String = NetworkConstants.WEATHER_BASE_URL ): WeatherRepository { val tokenManager = createTokenManager(tokenStorage, authRepository) val httpClient = createAuthenticatedHttpClient(tokenManager) val apiService = KtorWeatherApiService(httpClient, baseUrl) return WeatherRepositoryImpl(apiService, networkConnectivity) }Then update this provider:
- fun provideWeatherRepository( - networkConnectivity: NetworkConnectivity, - tokenStorage: TokenStorage - ): WeatherRepository { - return createWeatherRepository(networkConnectivity, tokenStorage) - } + fun provideWeatherRepository( + networkConnectivity: NetworkConnectivity, + tokenStorage: TokenStorage, + authRepository: AuthRepository + ): WeatherRepository { + return createWeatherRepository(networkConnectivity, authRepository, tokenStorage) + }storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt (2)
20-25: Prefer suspend DAO methods over withContext wrappers.Room supports suspend DAO APIs. Making DAO methods suspend lets you drop the withContext wrappers here, reducing boilerplate and the chance of accidentally blocking the main thread in future changes.
Apply after updating DAO to suspend:
override suspend fun saveToken(token: String) { - withContext(Dispatchers.IO) { - val authToken = AuthToken(token = token) - database.authTokenDao().saveToken(authToken) - } + val authToken = AuthToken(token = token) + database.authTokenDao().saveToken(authToken) }Outside this file, update DAO signatures to
suspend(see DAO comment).
38-42: LGTM: clear operation scoped to IO.Once DAO methods are suspend, you can remove
withContexthere too (mirroring thesaveTokensuggestion).storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthTokenDao.kt (2)
26-28: MakegetToken()suspend to avoid blocking callers.Non-suspend DAOs can be accidentally invoked from the main thread. Switching to
suspend fun getToken(): AuthToken?helps keep thread policy safe and lets callers dropwithContext.
40-41: Consider makingclearTokens()suspend and returning Unit.For consistency with other DAO methods and to discourage main-thread calls. Returning the deleted row count is rarely used for this table and can be omitted unless needed.
network/src/commonMain/kotlin/bose/ankush/network/auth/api/KtorAuthApiService.kt (1)
29-33: Optional: unify URL building and content type declarations.You’re mixing string interpolation for paths and
url {}for parameters. Consider consistently usingurl { path("login") },url { path("register") }, etc., and declaringcontentTypefor all endpoints for clarity.Also applies to: 37-42
network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt (4)
34-36: Throttle semantics can starve refreshes after failures. UpdatelastRefreshTimeonly on success.Currently,
lastRefreshTimeis set before the refresh attempt. If the call fails, subsequent refresh attempts are throttled for 5 minutes, potentially causing repeated 401s. Update it only after a successful refresh (or tracklastAttemptTimeseparately).Minimal change if you keep the
withLockapproach:- lastRefreshTime = currentTime try { val response = authRepository.refreshToken() ?: return null val newToken = response.data?.token if (response.status && !newToken.isNullOrBlank()) { tokenStorage.saveToken(newToken) + lastRefreshTime = currentTimeAlso applies to: 43-47
24-32: Consider expiry-aware refresh instead of time-based heuristics.
getValidTokenonly uses a time-based throttle and returns the current token even if it's expired. If your JWT has anexpclaim, decoding and preemptively refreshing when nearing expiry will reduce 401s and unnecessary retries.I can add a small JWT decoder (no crypto) to read
expand compute a safe preemptive refresh window. Want me to draft that?
15-19: DefaultdebugLoggingto false to avoid noisy logs in production.Safer default for production environments; enable explicitly when needed.
-class TokenManager( +class TokenManager( private val tokenStorage: TokenStorage, private val authRepository: AuthRepository, - private val debugLogging: Boolean = true + private val debugLogging: Boolean = false ) {
63-66: GuardlastRefreshTimeupdates for consistency.
lastRefreshTime = 0is updated outside the mutex. While low-risk, making all writes to the throttle state consistent (under the same lock or via atomic) reduces race potential.- suspend fun handleUnauthorized(): Boolean { - lastRefreshTime = 0 - return refreshToken() != null - } + suspend fun handleUnauthorized(): Boolean { + refreshMutex.lock() + try { + lastRefreshTime = 0 + } finally { + refreshMutex.unlock() + } + return refreshToken() != null + }network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (2)
69-87: Prefer Ktor’s Auth plugin (bearer) over customrunBlockingin defaultRequest.Since this function installs auth via
configureAuth(tokenManager), note that the current interceptor usesrunBlockingto get a token. This blocks Ktor threads and risks stalls. Ktor’sAuthplugin supports suspendable token loaders and refresh.Example (to implement inside your
configureAuthextension instead ofdefaultRequest+ResponseObserver):install(Auth) { bearer { loadTokens { tokenManager.getValidToken()?.let { BearerTokens(it, "") } } refreshTokens { tokenManager.refreshToken()?.let { BearerTokens(it, "") } } sendWithoutRequest { request -> true } // if every request needs auth } }Want me to adapt your
AuthInterceptor.ktaccordingly?
95-102: DRY: Extract acreateCommonJson()helper to avoid repeated JSON config blocks.You repeat identical Json config twice. Extracting a helper keeps it consistent.
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (1)
85-92: Externalize user-facing string (“Authentication successful”).Use a string resource for localization and consistency.
- // Show success message in glassmorphic snackbar - showSnackbar("Authentication successful") + // Show success message in glassmorphic snackbar + showSnackbar(context.getString(R.string.authentication_successful))Note: add
authentication_successfulto strings.xml if not present.network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepository.kt (2)
55-60: Clarify nullability contract for refresh.Document that null means “no token available to refresh” or “refresh endpoint failed/unavailable” to simplify caller handling.
31-41: Optional: prefer strong types for time/device info over Strings.Consider
Instant/Longfor timestamps and sealed/enum/typed wrappers for OS/versions. This improves validation and reduces downstream parsing.app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (4)
73-73: Remove unused rememberCoroutineScope().
rememberCoroutineScope()is called but not used.- rememberCoroutineScope()
396-475: Good logout UX; minor polish: localize strings and disable card while logging out.
- Dialog and card are well-structured. Replace hard-coded strings with
stringResource(...)for localization.- While
isLoggingOutdisables dialog buttons, consider disabling the card click too to avoid repeated dialog triggers during logout.Example changes (resource IDs illustrative):
Text(text = stringResource(R.string.logout)) Text(text = stringResource(R.string.logout_confirm_message)) TextButton(onClick = { viewModel.logout() }, enabled = !isLoggingOut) { Text(stringResource(R.string.confirm)) } TextButton(onClick = { showLogoutDialog.value = false }, enabled = !isLoggingOut) { Text(stringResource(R.string.cancel)) } // Card clickable: .clickable(enabled = !isLoggingOut) { showLogoutDialog.value = true }Also applies to: 477-495
159-191: Localize all user-visible strings (Notification, Turn on, Language, Get Premium, Sign out…).Multiple hard-coded strings should use
stringResource(...)for i18n and consistency.I can prepare a strings.xml patch with suggested keys/values if helpful.
Also applies to: 246-261, 329-345, 444-458
217-276: Accessibility: Use stringResource for contentDescription and improve semantics.Replace content descriptions with localized strings and consider adding semantics for clickable surfaces to aid screen readers.
Also applies to: 349-357, 466-472, 650-666
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (2)
125-130: Rename shadowing variable for clarityLocal variable message shadows the earlier message (now removed per prior suggestion), and is ambiguous. Rename to avoid confusion.
- val message = if (!errorMsg.isNullOrBlank()) { - errorMsg - } else { - response.message ?: "Logout failed" - } - Result.failure(Exception(message)) + val errorMessage = if (!errorMsg.isNullOrBlank()) { + errorMsg + } else { + response.message ?: "Logout failed" + } + Result.failure(Exception(errorMessage))
115-121: Rely solely on the response shape for logout success
Checkingresponse.datais sufficient to determine a successful logout—dropping the message‐text check makes this logic immune to localization or copy tweaks.• File:
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt
Lines 115–121Proposed diff:
- val message = response.message ?: "" - val isSuccess = response.data == null && ( - message.contains("Logout successful", ignoreCase = true) || - message.contains("Logged out successfully", ignoreCase = true) - ) + val isSuccess = response.data == nullnetwork/src/commonMain/kotlin/bose/ankush/network/auth/model/AuthModels.kt (1)
51-55: Avoid defaulting status to trueDefaulting status to true risks treating malformed/partial responses as successful if the field is missing. Prefer a safer default or make the field non-optional in your API contract.
Options:
- Change default to false, or
- Remove the default initializer so deserialization requires the field.
-@Serializable -data class AuthResponse( - val status: Boolean = true, +@Serializable +data class AuthResponse( + val status: Boolean = false, val message: String? = null, val data: AuthData? = null )If the backend always returns status, removing the default would be even stricter.
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (3)
345-355: Run auth calls off the main thread and use centralized error mappingLaunch on IO to keep the main thread free, and reuse errorResponseFromException for consistent UX and error semantics.
- fun login(email: String, password: String) { - viewModelScope.launch { + fun login(email: String, password: String) { + viewModelScope.launch(dispatchers.io) { _authState.value = AuthState.Loading try { val response = authRepository.login(email, password) handleAuthResponse(response) } catch (e: Exception) { - _authState.value = AuthState.Error(UiText.DynamicText(e.message ?: "Login failed")) + _authState.value = AuthState.Error(errorResponseFromException(e)) } } }
362-393: Same as login: move to IO and centralize error handling- fun register(email: String, password: String) { - viewModelScope.launch { + fun register(email: String, password: String) { + viewModelScope.launch(dispatchers.io) { _authState.value = AuthState.Loading try { // Collect device information val timestamp = Extension.getCurrentUtcTimestamp() val deviceModel = Extension.getDeviceModel() val operatingSystem = Extension.getOperatingSystem() val osVersion = Extension.getOsVersion() val appVersion = Extension.getAppVersion() val ipAddress = Extension.getIpAddress() val registrationSource = Extension.getRegistrationSource() // Call repository with enhanced data val response = authRepository.register( email = email, password = password, timestampOfRegistration = timestamp, deviceModel = deviceModel, operatingSystem = operatingSystem, osVersion = osVersion, appVersion = appVersion, ipAddress = ipAddress, registrationSource = registrationSource ) handleAuthResponse(response) } catch (e: Exception) { - _authState.value = - AuthState.Error(UiText.DynamicText(e.message ?: "Registration failed")) + _authState.value = AuthState.Error(errorResponseFromException(e)) } } }
398-416: Same as login/register: move to IO and unify error mapping (including Result failure)- fun logout() { - viewModelScope.launch { + fun logout() { + viewModelScope.launch(dispatchers.io) { _authState.value = AuthState.LogoutLoading try { val result = authRepository.logout() if (result.isSuccess) { _authState.value = AuthState.LoggedOut } else { - _authState.value = AuthState.Error( - UiText.DynamicText( - result.exceptionOrNull()?.message ?: "Logout failed" - ) - ) + val ex = result.exceptionOrNull() + val uiText = if (ex is Exception) { + errorResponseFromException(ex) + } else { + UiText.StringResource(R.string.general_error_txt) + } + _authState.value = AuthState.Error(uiText) } } catch (e: Exception) { - _authState.value = AuthState.Error(UiText.DynamicText(e.message ?: "Logout failed")) + _authState.value = AuthState.Error(errorResponseFromException(e)) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
app/build.gradle.kts(2 hunks)app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt(6 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt(9 hunks)app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt(6 hunks)app/src/test/java/bose/ankush/weatherify/base/DateTimeUtilsTest.kt(1 hunks)build.gradle.kts(2 hunks)buildSrc/src/main/java/Versions.kt(2 hunks)network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/api/AuthApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/api/KtorAuthApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/model/AuthModels.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepository.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/storage/TokenStorage.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/WeatherForecast.kt(1 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt(2 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt(1 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthToken.kt(1 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthTokenDao.kt(1 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherDatabase.kt(1 hunks)sunriseui/build.gradle.kts(1 hunks)sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt(1 hunks)sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sunriseui/build.gradle.kts
🧰 Additional context used
🧬 Code Graph Analysis (5)
app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt (1)
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (2)
createWeatherRepository(47-62)createAuthRepository(119-127)
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (3)
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt (1)
createPlatformHttpClient(16-39)network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt (1)
createPlatformHttpClient(16-37)network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt (2)
configureAuth(20-58)configureAuth(64-73)
network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt (4)
network/src/commonMain/kotlin/bose/ankush/network/auth/api/AuthApiService.kt (1)
refreshToken(32-32)network/src/commonMain/kotlin/bose/ankush/network/auth/api/KtorAuthApiService.kt (1)
refreshToken(45-59)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepository.kt (1)
refreshToken(59-59)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (1)
refreshToken(94-110)
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (1)
language/src/main/java/bose/ankush/language/presentation/LanguageScreen.kt (1)
ScreenHeader(140-186)
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (1)
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (1)
errorResponseFromException(50-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qodana
🔇 Additional comments (28)
buildSrc/src/main/java/Versions.kt (4)
8-8: Compose BOM upgrade: watch for skew with individual Compose artifactsAfter raising the BOM to 2025.05.01, double-check that all Compose dependencies (material3, ui, tooling, etc.) are pulled from the BOM and no hardcoded versions drift out of sync.
17-20: Good separation of ktlint Gradle plugin vs CLI versionSplitting the Gradle plugin version from the CLI version prevents resolution conflicts and works well with Spotless.
60-61: Hilt compiler alignment confirmed; please verify Compose BOM usage
- app/build.gradle.kts (around line 145) already applies
kapt(Deps.hiltDaggerAndroidCompiler)which matchesVersions.hilt = "2.56.2".- buildSrc/src/main/java/Dependencies.kt defines
val hiltNavigationCompose = "androidx.hilt:hilt-navigation-compose:1.2.0", but I didn’t detect any Compose BOM import in your Gradle files.Please confirm that you’re either:
- Importing the Compose BOM that bundles
hilt-navigation-compose:1.2.0, or- Explicitly managing
hilt-navigation-composeversion to ensure compatibility with your Compose dependencies.
5-5: Confirm AGP 8.11.1 Declaration and Compose Plugin Usage
- No remaining references to
Versions.kotlinCompiler.- Compose Gradle plugin (
org.jetbrains.kotlin.plugin.compose) is applied in:
• app/build.gradle.kts
• build.gradle.kts (root)
• language/build.gradle.kts
• payment/build.gradle.kts
• sunriseui/build.gradle.kts- I did not find an explicit AGP version (
com.android.tools.build:gradle:8.11.1) in:
• root build.gradle.kts (buildscript classpath)
• settings.gradle.kts pluginManagement block
• any version catalog (e.g.libs.versions.toml)Please verify that AGP 8.11.1 is declared in your buildscript or pluginManagement (or version catalog) and that all modules use the correct plugin version.
app/build.gradle.kts (2)
1-1: Import for ComposeFeatureFlag is correct for the Kotlin Compose Gradle pluginThis aligns with the new composeCompiler configuration style in Kotlin 2.x.
149-150: Lottie removal verified – no remaining API or Gradle references
Ran searches for common Lottie symbols in Kotlin sources and across all Gradle files; no usages were found. ThelottieComposeconstant remains inbuildSrcbut isn’t referenced in anybuild.gradle.kts.build.gradle.kts (6)
19-19: Ktlint plugin version reference updated to granular constantConsistent with the new Versions.kt layout. Looks good.
21-21: Detekt plugin added—sane defaults with ignoreFailuresThis is a good incremental adoption strategy without breaking CI.
53-79: Spotless+ktlint via CLI looks solid; verify editorConfig overrides with ktlint 1.3.xThe override key ktlint_function_naming_ignore_when_annotated_with should be supported, but it changed historically. If Spotless/ktlint warns about unknown property, adjust the key accordingly.
Run spotlessCheck locally and inspect the warning. If needed, I can propose the correct key for your ktlint version.
104-121: Aggregator tasks are helpful for CI/local workflowsNice touch with spotlessCheckAll/ApplyAll and detektAll; simplifies maintenance.
123-130: Convenience cleanup task is a good additionKeeps formatting-only changes separate. No issues.
12-24: Remove unused ktlint Gradle plugin declarationNo occurrences of
apply(plugin = "org.jlleitschuh.gradle.ktlint")were found in the codebase, which indicates the ktlint plugin isn’t actually applied anywhere. You can safely remove this line from yourbuild.gradle.ktsto reduce plugin noise:• In
build.gradle.kts, delete:- id("org.jlleitschuh.gradle.ktlint") version Versions.ktLintGradlePlugin apply falsenetwork/src/commonMain/kotlin/bose/ankush/network/auth/storage/TokenStorage.kt (1)
9-32: LGTM: clear multiplatform token storage abstractionThe API surface is minimal and pragmatic for JWT persistence across platforms. Naming and KDoc are clear.
sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt (1)
41-101: LGTM: tasteful glassmorphic snackbar with clean enter/exit animationsAnimation specs, gradient, elevation, and border look cohesive. The auto-dismiss behavior via LaunchedEffect is idiomatic.
app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (1)
80-129: PII/sensitive metadata: ensure consent and clear disclosureCollecting and transmitting device model, OS version, app version, timestamp, and IP address may have privacy implications depending on jurisdiction and your privacy policy. Ensure collection is:
- documented in your privacy policy,
- consented to where required,
- strictly necessary for the stated purpose,
- and securely transmitted/stored.
Would you like a short checklist or code examples (e.g., gating collection behind a runtime setting) to align this with your privacy posture?
storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherDatabase.kt (1)
15-15: LGTM: DAO exposure for AuthTokenExposing authTokenDao in the database is consistent with the new TokenStorage implementation.
storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt (1)
65-71: LGTM: TokenStorage provider wiringSingleton TokenStorage backed by Room looks correct and aligns DI with the new auth flow.
app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt (1)
49-58: LGTM: AuthRepository DI providerProviding AuthRepository via Hilt completes the auth wiring for UI flows that need to trigger login/refresh.
storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt (2)
33-36: LGTM: exposing Flow directly is appropriate.Returning the DAO’s Flow allows Room to manage threading and emissions; no extra
withContextneeded.
20-25: Single-row invariant confirmed in AuthTokenAuthToken.id is defined with a default value of 1 (
val id: Int = 1), matching the DAO’s hard-codedWHERE id = 1queries. No further changes are needed here.storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthTokenDao.kt (1)
33-34: LGTM: Room Flow for existence checks.Returning
Flow<Boolean>is efficient and avoids manual dispatching; Room manages query threads.network/src/commonMain/kotlin/bose/ankush/network/auth/api/AuthApiService.kt (1)
12-39: Interface looks solid and cohesive.Clear API surface for auth flows; names and docs are consistent. No issues.
network/src/commonMain/kotlin/bose/ankush/network/auth/api/KtorAuthApiService.kt (1)
61-65: Confirm that the HttpClient used here attaches auth headers for logout (if required by backend).
logout()relies on client-level auth configuration to send the Authorization header. If this service is constructed with a non-authenticated client, the call may fail with 401.
- Ensure DI provides an authenticated
HttpClientinstance for this service when callinglogout().- Alternatively, mirror the refresh-token approach and attach the token explicitly here as well.
I can help inspect DI wiring if you want me to verify which client instance is injected.
network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt (1)
41-58: Confirm kotlinx-coroutines version or switch to explicit locking
We couldn’t locate an explicitkotlinx-coroutinesdependency declaration in your Gradle files (build scripts, version catalogs or BOM). SinceMutex.withLockonly accepts a non-suspending block on older coroutine releases, holding a mutex across a network call can lead to deadlocks if the lambda ever suspends.Please:
- Verify which
kotlinx-coroutinesversion your project is using (checkbuild.gradle(.kts), anylibs.versions.tomlcatalogs or BOM imports).- If your version doesn’t support suspendable
withLocklambdas, refactor to manuallock()/unlock()as shown below.- Also, move the
lastRefreshTime = currentTimeassignment inside the success branch so it only updates on a successful refresh.Proposed refactor:
- suspend fun refreshToken(currentTime: Long = Clock.System.now().epochSeconds): String? = - refreshMutex.withLock { - lastRefreshTime = currentTime + suspend fun refreshToken(currentTime: Long = Clock.System.now().epochSeconds): String? { + refreshMutex.lock() try { - val response = authRepository.refreshToken() ?: return null + val response = authRepository.refreshToken() ?: return null val newToken = response.data?.token if (response.status && !newToken.isNullOrBlank()) { - tokenStorage.saveToken(newToken) - if (debugLogging) println("[DEBUG_LOG] Token refreshed successfully") - return newToken + tokenStorage.saveToken(newToken) + lastRefreshTime = currentTime + if (debugLogging) println("[DEBUG_LOG] Token refreshed successfully") + return newToken } else { if (debugLogging) println("[DEBUG_LOG] Token refresh failed: Invalid response") } } catch (e: Exception) { if (debugLogging) println("[DEBUG_LOG] Token refresh failed: ${e.message}") } finally { - return null - } + refreshMutex.unlock() + } + return null }app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (1)
69-83: Nice integration of auth state and non-intrusive feedback.The glassmorphic snackbar and state-reset loop are cleanly wired and won’t block UI flow.
Also applies to: 170-176
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (1)
99-107: Header simplification looks clean and consistent with the app style.The new
ScreenHeadersignature and animation are straightforward and easy to reuse.app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (2)
117-129: Auth initialization flow wiring looks solidSubscribing to isLoggedIn() and gating isAuthInitialized on first emission is clean and avoids UI flicker. No issues spotted.
422-431: Auth response handling logic is reasonableMapping success to a non-blank token and status true is pragmatic and aligns with repository behavior.
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt
Outdated
Show resolved
Hide resolved
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt
Outdated
Show resolved
Hide resolved
| override suspend fun refreshToken(request: RefreshTokenRequest): AuthResponse { | ||
| return NetworkUtils.retryWithExponentialBackoff { | ||
| httpClient.post("$baseUrl/refresh-token") { | ||
| // Pass token as a query parameter as per new API contract | ||
| url { parameters.append("token", request.token) } | ||
|
|
||
| // Optionally include the current token in Authorization header if present | ||
| tokenStorage?.getToken()?.let { token -> | ||
| if (token.isNotBlank()) { | ||
| header("Authorization", "Bearer $token") | ||
| } | ||
| } | ||
| }.body() | ||
| } |
There was a problem hiding this comment.
Suspend call inside request builder will not compile; fetch token before building the request.
TokenStorage.getToken() is suspend; calling it inside the post { ... } builder (non-suspend) won’t compile and can’t run. Pre-fetch the token in the suspending context, then set the header in the builder.
Apply this diff:
override suspend fun refreshToken(request: RefreshTokenRequest): AuthResponse {
return NetworkUtils.retryWithExponentialBackoff {
- httpClient.post("$baseUrl/refresh-token") {
+ val currentToken = tokenStorage?.getToken()
+ httpClient.post("$baseUrl/refresh-token") {
// Pass token as a query parameter as per new API contract
url { parameters.append("token", request.token) }
// Optionally include the current token in Authorization header if present
- tokenStorage?.getToken()?.let { token ->
- if (token.isNotBlank()) {
- header("Authorization", "Bearer $token")
- }
- }
+ if (!currentToken.isNullOrBlank()) {
+ header("Authorization", "Bearer $currentToken")
+ }
}.body()
}
}📝 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.
| override suspend fun refreshToken(request: RefreshTokenRequest): AuthResponse { | |
| return NetworkUtils.retryWithExponentialBackoff { | |
| httpClient.post("$baseUrl/refresh-token") { | |
| // Pass token as a query parameter as per new API contract | |
| url { parameters.append("token", request.token) } | |
| // Optionally include the current token in Authorization header if present | |
| tokenStorage?.getToken()?.let { token -> | |
| if (token.isNotBlank()) { | |
| header("Authorization", "Bearer $token") | |
| } | |
| } | |
| }.body() | |
| } | |
| override suspend fun refreshToken(request: RefreshTokenRequest): AuthResponse { | |
| return NetworkUtils.retryWithExponentialBackoff { | |
| // Pre-fetch the potentially suspend token | |
| val currentToken = tokenStorage?.getToken() | |
| httpClient.post("$baseUrl/refresh-token") { | |
| // Pass token as a query parameter as per new API contract | |
| url { parameters.append("token", request.token) } | |
| // Optionally include the current token in Authorization header if present | |
| if (!currentToken.isNullOrBlank()) { | |
| header("Authorization", "Bearer $currentToken") | |
| } | |
| }.body() | |
| } | |
| } |
| defaultRequest { | ||
| runBlocking { | ||
| val token = tokenManager.getValidToken() | ||
| if (!token.isNullOrBlank()) { | ||
| header("Authorization", "Bearer $token") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid runBlocking in KMP; switch to suspendable Ktor interceptors and retry.
Using runBlocking inside client configuration blocks risks deadlocks and UI freezes, especially in KMP (Android/iOS/JS). Also, the current flow refreshes on 401 but does not retry the failed request.
Refactor to:
- Use
HttpSendintercept (suspend) to attach the token. - Use
HttpRequestRetryto refresh on 401 and retry once with the new token. - Drop the redundant
HttpResponseValidatorthat just rethrows.
Apply this diff to the function body:
fun HttpClientConfig<*>.configureAuth(tokenManager: TokenManager) {
- // Add authorization header to all requests
- defaultRequest {
- runBlocking {
- val token = tokenManager.getValidToken()
- if (!token.isNullOrBlank()) {
- header("Authorization", "Bearer $token")
- }
- }
- }
-
- // Handle 401 Unauthorized responses by refreshing the token
- HttpResponseValidator {
- handleResponseExceptionWithRequest { exception, request ->
- // Re-throw the exception to let the caller handle it
- throw exception
- }
- }
-
- // Add response observer to handle 401 responses
- install(ResponseObserver) {
- onResponse { response ->
- if (response.status == HttpStatusCode.Unauthorized) {
- // Log the 401 response
- println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}")
-
- // Attempt to refresh the token
- runBlocking {
- val refreshed = tokenManager.handleUnauthorized()
- if (refreshed) {
- println("[DEBUG_LOG] Token refreshed successfully after 401 response")
- } else {
- println("[DEBUG_LOG] Failed to refresh token after 401 response")
- }
- }
- }
- }
- }
+ // Attach token in a suspend-friendly way
+ install(io.ktor.client.plugins.HttpSend) {
+ intercept { request ->
+ val token = tokenManager.getValidToken()
+ if (!token.isNullOrBlank()) {
+ request.headers.append(io.ktor.http.HttpHeaders.Authorization, "Bearer $token")
+ }
+ execute(request)
+ }
+ }
+
+ // Refresh on 401 and retry once with updated token
+ install(io.ktor.client.plugins.HttpRequestRetry) {
+ retryIf(maxRetries = 1) { _, response ->
+ response.status == HttpStatusCode.Unauthorized
+ }
+ modifyRequest { request ->
+ if (tokenManager.handleUnauthorized()) {
+ val newToken = tokenManager.getValidToken()
+ request.headers.remove(io.ktor.http.HttpHeaders.Authorization)
+ if (!newToken.isNullOrBlank()) {
+ request.headers.append(io.ktor.http.HttpHeaders.Authorization, "Bearer $newToken")
+ }
+ }
+ }
+ }
}Add imports (outside this hunk):
- io.ktor.client.plugins.HttpSend
- io.ktor.client.plugins.HttpRequestRetry
- io.ktor.http.HttpHeaders
Run this to confirm the Ktor version supports HttpRequestRetry (Ktor 2.0+):
Also applies to: 31-37, 39-57
🏁 Script executed:
#!/bin/bash
fd -a -t f -E '**/build/**' -E '**/.gradle/**' 'build.gradle*' 'gradle.properties' 'libs.versions.toml'
echo "----"
rg -nC2 -i 'ktor|io\.ktor' --no-ignoreLength of output: 15827
Switch to suspendable Ktor interceptors and add retry on 401
Avoid runBlocking in KMP client setup (can deadlock on Android/iOS) and implement a true retry flow. Replace the current defaultRequest + HttpResponseValidator+ ResponseObserver approach in:
network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt(lines 22–57)
with suspend-friendly plugins:
fun HttpClientConfig<*>.configureAuth(tokenManager: TokenManager) {
- // Add authorization header to all requests
- defaultRequest {
- runBlocking {
- val token = tokenManager.getValidToken()
- if (!token.isNullOrBlank()) {
- header("Authorization", "Bearer $token")
- }
- }
- }
-
- // Handle 401 Unauthorized responses by refreshing the token
- HttpResponseValidator {
- handleResponseExceptionWithRequest { exception, _ ->
- throw exception
- }
- }
-
- // Add response observer to handle 401 responses
- install(ResponseObserver) {
- onResponse { response ->
- if (response.status == HttpStatusCode.Unauthorized) {
- println("[DEBUG_LOG] 401 from ${response.request.url}")
- runBlocking { tokenManager.handleUnauthorized() }
- }
- }
- }
+ // Attach token in a suspend-friendly way
+ install(io.ktor.client.plugins.HttpSend) {
+ intercept { request ->
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { token ->
+ request.headers.append(io.ktor.http.HttpHeaders.Authorization, "Bearer $token")
+ }
+ execute(request)
+ }
+ }
+
+ // On 401, refresh token and retry once
+ install(io.ktor.client.plugins.HttpRequestRetry) {
+ retryIf(maxRetries = 1) { _, response ->
+ response.status == HttpStatusCode.Unauthorized
+ }
+ modifyRequest { request ->
+ if (tokenManager.handleUnauthorized()) {
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { newToken ->
+ request.headers.remove(io.ktor.http.HttpHeaders.Authorization)
+ request.headers.append(io.ktor.http.HttpHeaders.Authorization, "Bearer $newToken")
+ }
+ }
+ }
+ }
}Add at top of file:
import io.ktor.client.plugins.HttpSend
import io.ktor.client.plugins.HttpRequestRetry
import io.ktor.http.HttpHeaders🤖 Prompt for AI Agents
In
network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt
around lines 22 to 57, remove the runBlocking-based defaultRequest and the
current HttpResponseValidator/ResponseObserver pattern and replace them with
suspendable Ktor plugins: add imports for io.ktor.client.plugins.HttpSend,
io.ktor.client.plugins.HttpRequestRetry and io.ktor.http.HttpHeaders, register
an HttpSend plugin to suspendably obtain tokenManager.getValidToken() and set
the Authorization header on the outgoing request, and register HttpRequestRetry
to detect 401 responses, call a suspend token refresh (e.g.,
tokenManager.refreshToken()) and retry the original request once (guarding to
avoid infinite retries), ensuring thread-safety for tokenManager and removing
any blocking calls.
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt
Show resolved
Hide resolved
| // Create a platform-specific HttpClient with authentication configuration | ||
| return HttpClient(createPlatformHttpClient(json).engine) { | ||
| // Install ContentNegotiation plugin | ||
| install(ContentNegotiation) { | ||
| json(json) | ||
| } | ||
|
|
||
| // Add authentication configuration with token refresh | ||
| configureAuth(tokenManager) | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t create a throwaway HttpClient just to steal its engine — this leaks resources and drops platform plugins. Use client.config {} to extend the platform client.
Constructing a client via HttpClient(createPlatformHttpClient(json).engine) allocates an intermediate client that’s never closed and discards the platform-installed plugins (timeouts, Logging, ContentNegotiation variants). This can leak the original client and misconfigure the final client.
Use the platform client and extend it with additional config:
- // Create a platform-specific HttpClient with authentication configuration
- return HttpClient(createPlatformHttpClient(json).engine) {
- // Install ContentNegotiation plugin
- install(ContentNegotiation) {
- json(json)
- }
-
- // Add authentication configuration with token refresh
- configureAuth(tokenManager)
- }
+ // Start from platform client and extend it (preserves engine and plugins)
+ val base = createPlatformHttpClient(json)
+ return base.config {
+ // Only add auth here; ContentNegotiation is already configured by platform client
+ configureAuth(tokenManager)
+ }📝 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.
| // Create a platform-specific HttpClient with authentication configuration | |
| return HttpClient(createPlatformHttpClient(json).engine) { | |
| // Install ContentNegotiation plugin | |
| install(ContentNegotiation) { | |
| json(json) | |
| } | |
| // Add authentication configuration with token refresh | |
| configureAuth(tokenManager) | |
| } | |
| } | |
| // Start from platform client and extend it (preserves engine and plugins) | |
| val base = createPlatformHttpClient(json) | |
| return base.config { | |
| // Only add auth here; ContentNegotiation is already configured by platform client | |
| configureAuth(tokenManager) | |
| } |
🤖 Prompt for AI Agents
In network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt around
lines 77 to 87, you are creating a throwaway HttpClient by calling
HttpClient(createPlatformHttpClient(json).engine) which leaks the intermediate
client and drops platform-installed plugins; instead obtain the platform client
once (val platformClient = createPlatformHttpClient(json)) and call
platformClient.config { ... } to install ContentNegotiation/json and apply
configureAuth(tokenManager), then return that configured client so you don’t
allocate or leak an extra client or lose platform plugins.
| // Create a platform-specific HttpClient with authentication configuration | ||
| return HttpClient(createPlatformHttpClient(json).engine) { | ||
| // Install ContentNegotiation plugin | ||
| install(ContentNegotiation) { | ||
| json(json) | ||
| } | ||
|
|
||
| // Add authentication configuration | ||
| configureAuth(tokenStorage) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue for the legacy overload — avoid engine reuse via a temporary client.
This repeats the engine hijacking/leak. Extend the platform client instead.
- // Create a platform-specific HttpClient with authentication configuration
- return HttpClient(createPlatformHttpClient(json).engine) {
- // Install ContentNegotiation plugin
- install(ContentNegotiation) {
- json(json)
- }
-
- // Add authentication configuration
- configureAuth(tokenStorage)
- }
+ val base = createPlatformHttpClient(json)
+ return base.config {
+ // Add authentication configuration
+ configureAuth(tokenStorage)
+ }🤖 Prompt for AI Agents
In network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt around
lines 103 to 113, the current code constructs a new HttpClient by passing
createPlatformHttpClient(json).engine which reuses and effectively hijacks the
temporary client's engine; change the pattern so you extend/configure the
platform-specific client instance itself instead of creating a new one from its
engine. Specifically, call createPlatformHttpClient(json) to obtain the actual
platform client and then install ContentNegotiation and apply
configureAuth(tokenStorage) on that client instance (and do the same update for
the legacy overload) so no engine is reused from a temporary client.
| data class Data( | ||
| @SerialName("alerts") | ||
| val alerts: List<Alert?>? = null, | ||
| @SerialName("current") | ||
| val current: Current?, | ||
| @SerialName("daily") | ||
| val daily: List<Daily?>?, | ||
| @SerialName("hourly") | ||
| val hourly: List<Hourly?>? | ||
| ) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Nesting Alert/Current/Daily/Hourly under Data is a breaking API change; verify and update all references
Moving these types under WeatherForecast.Data will break any code referencing WeatherForecast.Alert (etc.). Ensure all usages across app, storage, and UI now reference WeatherForecast.Data.Alert, WeatherForecast.Data.Current, etc.
Run this to locate old vs new references:
Also applies to: 39-46
🏁 Script executed:
#!/bin/bash
# Find references to old vs new nested types
rg -nP -C2 '\bWeatherForecast\.Alert\b|\bWeatherForecast\.Current\b|\bWeatherForecast\.Daily\b|\bWeatherForecast\.Hourly\b|\bWeatherForecast\.Data\.Alert\b|\bWeatherForecast\.Data\.Current\b|\bWeatherForecast\.Data\.Daily\b|\bWeatherForecast\.Data\.Hourly\b' --type kotlinLength of output: 10113
Update nested type references to match new WeatherForecast.Data structure
The types Alert, Current, Daily, and Hourly have been moved under WeatherForecast.Data. All occurrences of the old top‐level nested types must be updated to the new paths, otherwise the build will break.
Affected locations:
- UI components (
app/src/main/java/bose/ankush/weatherify/presentation/home/component/):- WeatherAlertLayout.kt:
List<WeatherForecast.Alert?>? - CurrentWeatherReportLayout.kt: all
WeatherForecast.Currentusages - HourlyWeatherForecastReportLayout.kt:
List<WeatherForecast.Hourly?> - DailyWeatherForecastReportLayout.kt:
List<WeatherForecast.Daily?>
- WeatherAlertLayout.kt:
- Data mapper (
app/src/main/java/bose/ankush/weatherify/data/mapper/WeatherMapper.kt):- Instantiations of
WeatherForecast.Alert,WeatherForecast.Current,WeatherForecast.Daily,WeatherForecast.Daily.Temp,WeatherForecast.Hourly
- Instantiations of
Action items:
- Replace every
WeatherForecast.Alert→WeatherForecast.Data.Alert(and similarly for Current, Daily, Daily.Temp, Hourly) - Update corresponding imports
- (Optional) Add typealiases in WeatherForecast.kt for backwards compatibility:
typealias Alert = Data.Alert typealias Current = Data.Current // etc.
| @Insert(onConflict = OnConflictStrategy.REPLACE) | ||
| fun saveToken(token: AuthToken): Long | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enforce single-row semantics at the DAO layer to avoid subtle bugs.
@Insert(REPLACE) with an entity that implicitly uses id = 1 relies on the entity’s default. If that default changes, getToken()/hasToken() (which filter id = 1) will break.
Two robust options:
- Use an explicit deterministic upsert with
id = 1:
@Query("INSERT OR REPLACE INTO auth_tokens(id, token) VALUES (1, :token)")
suspend fun saveToken(token: String)- Or keep entity-based API but require callers to pass
id = 1, and make this method suspend:
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun saveToken(token: AuthToken): LongEither way, prefer a suspend signature and remove the hardcoding dependency from callers.
🏁 Script executed:
#!/bin/bash
# Locate the AuthToken entity definition
rg -nP --glob 'storage/src/androidMain/kotlin/**/*.kt' 'data class AuthToken' -C5
# Locate getToken() DAO method to see its query filter
rg -nP --glob 'storage/src/androidMain/kotlin/**/*.kt' '\bfun getToken' -C5
# Locate hasToken() DAO method to see its query filter
rg -nP --glob 'storage/src/androidMain/kotlin/**/*.kt' '\bfun hasToken' -C5Length of output: 5921
Enforce deterministic single-row upsert in AuthTokenDao
Currently, saveToken(@Insert REPLACE) relies on the AuthToken entity’s default id = 1. If that default ever changes, both getToken() and hasToken() (which hard-code WHERE id = 1) will break. Refactor to pin the ID at the DAO layer and switch to a suspend signature:
Files to update:
- storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthTokenDao.kt
- (optionally) storage/src/androidMain/kotlin/bose/ankush/storage/room/AuthToken.kt
- storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt
Choose one of the following patterns:
-
Explicit upsert query
@Dao interface AuthTokenDao { @Query(""" INSERT OR REPLACE INTO auth_tokens(id, token) VALUES (1, :token) """) suspend fun saveToken(token: String) }
– No more reliance on the entity default.
– UpdategetToken()/hasToken()remain unchanged. -
Entity-based insert with explicit ID
@Entity(tableName = "auth_tokens") data class AuthToken( @PrimaryKey val id: Int, val token: String, val createdAt: Long = System.currentTimeMillis() ) @Dao interface AuthTokenDao { @Insert(onConflict = OnConflictStrategy.REPLACE) suspend fun saveToken(token: AuthToken): Long }
– Remove the
= 1default onAuthToken.idto force callers to passid = 1.
– UpdateTokenStorageImpl:override suspend fun saveToken(token: String) { val authToken = AuthToken(id = 1, token = token) database.authTokenDao().saveToken(authToken) }
Both options ensure a fixed ID at the DAO layer and leverage Room’s coroutine support for non-blocking I/O.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
18-19: Add a short “Permissions & Privacy” section for Location-based WeatherSince the app now uses location, consider documenting required permissions and privacy at a high-level. This helps users (and reviewers) understand data handling and permission prompts.
Here’s a ready-to-paste section you can add near “Setup & Installation” or below it:
## 🔒 Permissions & Privacy - Location: Used to fetch weather for your current area (coarse or precise based on your selection). - We do not store your precise location; it is used transiently to request weather data. - You can disable location at any time and manually select a city. For more details, see our Privacy Policy (add link) and Terms (add link).
60-64: Typo in Data Flow diagram: “Androidplay” → likely “OpenWeather API”Minor doc polish: the leftmost box label reads “Androidplay API,” which seems unintended.
Apply this minimal diff to fix the label:
-┌───────────────┐ API Data ┌───────────────┐ Network ┌───────────────┐ -│ │────────────────> │ │────────────────>│ │ -│ Androidplay │ │ Network │ │ Network │ -│ API │ │ Module │ │ Repository │ +┌───────────────┐ API Data ┌───────────────┐ Network ┌───────────────┐ +│ │────────────────> │ │────────────────>│ │ +│ OpenWeather │ │ Network │ │ Network │ +│ API │ │ Module │ │ Repository │
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...therify/releases/latest) ## 📱 Features - Current Weather: View today's temperatu...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qodana
🔇 Additional comments (2)
README.md (2)
9-9: APK download badge/link looks goodThe badge renders correctly and points to the latest release. No issues from my side.
14-14: All “4-Day” references removed
A global regex search for4-day,next 4 days, and4 days(excluding build artifacts) returned no matches. No lingering verbiage in UI copy, docs, or comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (3)
50-104: Simplify openAppLocaleSettings primary path; avoid building LOCALE intent twiceMinor cleanup: only build the per-app locale intent on Android 13+; keep the existing fallbacks unchanged.
Apply this diff:
- @SuppressLint("QueryPermissionsNeeded") - fun Context.openAppLocaleSettings() { - // Try opening the per-app language settings if available, otherwise fall back safely - val pm = packageManager - // Primary: Per-app language settings (Android 13+) - val appLocaleIntent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { - Intent(Settings.ACTION_APP_LOCALE_SETTINGS).apply { - data = Uri.fromParts("package", packageName, null) - addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) - } - } else { - Intent(Settings.ACTION_LOCALE_SETTINGS).apply { - addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) - } - } - - try { - val canHandleAppLocale = appLocaleIntent.resolveActivity(pm) != null - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU && canHandleAppLocale) { - startActivity(appLocaleIntent) - return - } - } catch (_: Exception) { - // Ignore and try fallbacks - } + @SuppressLint("QueryPermissionsNeeded") + fun Context.openAppLocaleSettings() { + // Try opening the per-app language settings if available, otherwise fall back safely + val pm = packageManager + // Primary: Per-app language settings (Android 13+). For older versions, skip to fallbacks. + val appLocaleIntent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + Intent(Settings.ACTION_APP_LOCALE_SETTINGS).apply { + data = Uri.fromParts("package", packageName, null) + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + } + } else null + + try { + if (appLocaleIntent != null && appLocaleIntent.resolveActivity(pm) != null) { + startActivity(appLocaleIntent) + return + } + } catch (_: Exception) { + // Ignore and try fallbacks + }
129-136: Nit: Doc example mentions iPhone; this module targets AndroidAdjust the comment example to avoid iOS references.
Apply this diff:
- /** - * Gets the device model (e.g., "Pixel 7 Pro", "iPhone 15") + /** + * Gets the device model (e.g., "Pixel 7 Pro")
179-201: Prefer backend-derived IP; if kept, constrain to IPv4 global first and fall back to privateCollecting device IP on the client usually yields private or IPv6 addresses and can be considered PII. Ideally, let the server derive it from the request. If you must keep this, prefer global IPv4 first, then fall back to site-local; also avoid returning link-local/loopback.
Apply this diff:
- fun getIpAddress(): String? { - try { - val networkInterfaces = NetworkInterface.getNetworkInterfaces() + fun getIpAddress(): String? { + var siteLocal: String? = null + try { + val networkInterfaces = NetworkInterface.getNetworkInterfaces() while (networkInterfaces.hasMoreElements()) { val networkInterface = networkInterfaces.nextElement() val inetAddresses = networkInterface.inetAddresses while (inetAddresses.hasMoreElements()) { val inetAddress = inetAddresses.nextElement() - if (!inetAddress.isLoopbackAddress && !inetAddress.isLinkLocalAddress) { - return inetAddress.hostAddress - } + if (!inetAddress.isLoopbackAddress && !inetAddress.isLinkLocalAddress && inetAddress is java.net.Inet4Address) { + val host = inetAddress.hostAddress + if (!inetAddress.isSiteLocalAddress) return host + if (siteLocal == null) siteLocal = host + } } } } catch (_: Exception) { // Ignore exceptions and return null } - return null + return siteLocal }Follow-ups:
- Do you actually need to send an IP? If yes, is user consent and the privacy policy updated accordingly? I can help remove this field from the request and have the backend derive it instead.
- Ensure this is not called on a performance-critical UI path; interface enumeration can be relatively slow on some devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: qodana
🔇 Additional comments (6)
app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (6)
3-3: LGTM: new imports match added functionalityThe introduced imports are necessary for the new utilities and locale-settings logic. No unused imports detected.
Also applies to: 12-17
137-144: LGTM: getOperatingSystem()Static "Android" is fine and matches the intended usage.
145-152: LGTM: getOsVersion()Using Build.VERSION.RELEASE is acceptable for a human-readable OS version string.
153-160: LGTM: getAppVersion()Returning BuildConfig.VERSION_NAME is the right choice for display/telemetry.
161-170: LGTM: getCurrentUtcTimestamp()Correct ISO-8601 formatting with UTC timezone and fixed locale. Suitable for server-side parsing.
171-178: LGTM: getRegistrationSource()Constant source string is fine for current needs.
| /** | ||
| * Opens a URL in the device's default browser | ||
| * @param url The URL to open | ||
| */ | ||
| fun Context.openUrlInBrowser(url: String) { | ||
| val intent = Intent(Intent.ACTION_VIEW, url.toUri()) | ||
| startActivity(intent) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent crashes in openUrlInBrowser for non-Activity contexts and handle invalid/unsupported URLs
Starting an activity from a non-Activity Context without FLAG_ACTIVITY_NEW_TASK can crash. Also, guard against malformed URLs and absent handlers.
Apply this diff:
- fun Context.openUrlInBrowser(url: String) {
- val intent = Intent(Intent.ACTION_VIEW, url.toUri())
- startActivity(intent)
- }
+ fun Context.openUrlInBrowser(url: String) {
+ // Ensure a scheme; default to https if missing
+ val safeUri = (if ("://" in url) url else "https://$url").toUri()
+ val intent = Intent(Intent.ACTION_VIEW, safeUri).apply {
+ if (this@openUrlInBrowser !is android.app.Activity) {
+ addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
+ }
+ }
+ try {
+ if (intent.resolveActivity(packageManager) != null) {
+ startActivity(intent)
+ }
+ } catch (_: Exception) {
+ // No-op: avoid crash if no browser is available
+ }
+ }📝 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.
| /** | |
| * Opens a URL in the device's default browser | |
| * @param url The URL to open | |
| */ | |
| fun Context.openUrlInBrowser(url: String) { | |
| val intent = Intent(Intent.ACTION_VIEW, url.toUri()) | |
| startActivity(intent) | |
| } | |
| /** | |
| * Opens a URL in the device's default browser | |
| * @param url The URL to open | |
| */ | |
| fun Context.openUrlInBrowser(url: String) { | |
| // Ensure a scheme; default to https if missing | |
| val safeUri = (if ("://" in url) url else "https://$url").toUri() | |
| val intent = Intent(Intent.ACTION_VIEW, safeUri).apply { | |
| // If this Context isn't an Activity, we need the NEW_TASK flag | |
| if (this@openUrlInBrowser !is android.app.Activity) { | |
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | |
| } | |
| } | |
| try { | |
| // Only start if there's an app to handle the Intent | |
| if (intent.resolveActivity(packageManager) != null) { | |
| startActivity(intent) | |
| } | |
| } catch (_: Exception) { | |
| // No-op: avoid crash if no browser is available or URL is malformed | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt around
lines 33 to 41, the current openUrlInBrowser starts an activity directly and can
crash for non-Activity Contexts or for invalid/unsupported URLs; modify it to
(1) validate and normalize the URL string (return early if blank or fails to
parse), (2) build the VIEW intent and, if the Context is not an Activity, add
Intent.FLAG_ACTIVITY_NEW_TASK, (3) check packageManager.resolveActivity(intent,
0) (or intent.resolveActivity(packageManager)) before calling startActivity, and
(4) wrap startActivity in a try/catch to safely handle any
ActivityNotFoundException or parsing errors and avoid crashes, logging or
failing silently as appropriate.
…Auth models
This commit introduces Firebase Cloud Messaging (FCM) token collection during user registration and updates the authentication data models. It also includes UI adjustments for status bar transparency and layout padding.
- **Authentication Enhancements:**
- `RegisterRequest` and `AuthRepository.register` now include `firebaseToken`.
- `MainViewModel` retrieves the FCM token using a new `Extension.getFirebaseToken()` suspending function and includes it in the registration request.
- `AuthData` model now includes `role`, `isActive`, and `isPremium` fields.
- `AuthResponse` model now includes a `success` field and an `isSuccess()` method to reliably check the outcome of authentication calls, preferring `success` over `status`.
- `AuthRepositoryImpl` and `TokenManager` updated to use `response.isSuccess()` for checking authentication and token refresh success.
- **UI Updates (MainActivity):**
- The status bar is now set to transparent.
- System UI controller is used to adjust status bar icon colors (dark/light) based on the background color's luminance.
- `windowInsetsPadding` is applied to the main content Box to respect safe drawing areas, particularly for the bottom and horizontal sides.
- **Build & Dependencies:**
- Added `com.google.firebase:firebase-messaging-ktx` dependency.
- Updated `app/build.gradle.kts` to use `androidResources.localeFilters` instead of the deprecated `resourceConfigurations`.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (1)
219-233: Render a single permission dialog at a time to avoid stacked overlaysMapping the whole queue with
forEachrenders multipleAlertDialogs simultaneously. Show only the latest (top-of-stack) item.- permissionQueue.reversed().forEach { permission -> - PermissionAlertDialog( - permissionTextProvider = when (permission) { - Manifest.permission.ACCESS_FINE_LOCATION -> FineLocationPermissionTextProvider() - Manifest.permission.ACCESS_COARSE_LOCATION -> CoarseLocationPermissionTextProvider() - else -> return@forEach - }, - isPermanentlyDeclined = shouldShowRequestPermissionRationale(permission), - onDismissClick = viewModel::dismissDialog, - onOkClick = { - viewModel.dismissDialog() - locationPermissionsResultLauncher.launch(PERMISSIONS_TO_REQUEST) - }, - onGoToAppSettingClick = { context.openAppSystemSettings() }) - } + permissionQueue.reversed().firstOrNull()?.let { permission -> + PermissionAlertDialog( + permissionTextProvider = when (permission) { + Manifest.permission.ACCESS_FINE_LOCATION -> FineLocationPermissionTextProvider() + Manifest.permission.ACCESS_COARSE_LOCATION -> CoarseLocationPermissionTextProvider() + else -> return@let + }, + isPermanentlyDeclined = !shouldShowRequestPermissionRationale(permission), + onDismissClick = viewModel::dismissDialog, + onOkClick = { + viewModel.dismissDialog() + locationPermissionsResultLauncher.launch(PERMISSIONS_TO_REQUEST) + }, + onGoToAppSettingClick = { context.openAppSystemSettings() } + ) + }app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (1)
170-179: Fix permission handling: avoid fetching location when permission is denied but already queuedThe current
elsebranch runs even when permission is denied but already present in the queue, causing location fetch without permission.fun onPermissionResult( permission: String, isGranted: Boolean, ) { - if (!isGranted && !permissionDialogQueue.contains(permission)) { - permissionDialogQueue.add(permission) - } else { - fetchAndSaveLocationCoordinates() - } + if (isGranted) { + fetchAndSaveLocationCoordinates() + } else if (!permissionDialogQueue.contains(permission)) { + permissionDialogQueue.add(permission) + } }
♻️ Duplicate comments (4)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (2)
139-147: Move location fetch into LaunchedEffect keyed by permission to avoid recomposition-triggered callsCalling
viewModel.fetchAndSaveLocationCoordinates()directly in composition can fire multiple times on recompositions. Gate it by permission and trigger viaLaunchedEffect. This mirrors the earlier feedback.- val launchNotificationPermissionState = - viewModel.launchNotificationPermission.collectAsState() - if (locationClient.hasLocationPermission()) { - // if permission granted already then fetch and save location coordinates - viewModel.fetchAndSaveLocationCoordinates() - } else { - // request location permission - RequestLocationPermission(context) - } + val launchNotificationPermissionState = + viewModel.launchNotificationPermission.collectAsState() + val hasLocationPermission = locationClient.hasLocationPermission() + LaunchedEffect(hasLocationPermission) { + if (hasLocationPermission) { + // fetch and save location coordinates once permission is granted + viewModel.fetchAndSaveLocationCoordinates() + } + } + if (!hasLocationPermission) { + // request location permission + RequestLocationPermission(context) + }
157-163: Key LaunchedEffect on the value, not the State object
LaunchedEffect(key1 = launchNotificationPermissionState)will not re-run when the underlying value changes. Use.value. This was flagged earlier as well.- LaunchedEffect(key1 = launchNotificationPermissionState) { + LaunchedEffect(key1 = launchNotificationPermissionState.value) { if (!context.hasNotificationPermission()) { viewModel.updateShowNotificationBannerState(true) } else { viewModel.updateShowNotificationBannerState(false) } }network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (1)
39-47: Remove raw token logging; redact and/or gate behind debug flagTokens are logged in full. This is a high-severity leakage risk. Remove raw values; keep generic messages or redacted snippets if necessary.
private suspend fun verifyTokenSaved(originalToken: String) { val savedToken = tokenStorage.getToken() if (savedToken != originalToken) { - println("[DEBUG_LOG] Token verification failed: Original token does not match saved token") - println("[DEBUG_LOG] Original: $originalToken") - println("[DEBUG_LOG] Saved: $savedToken") + println("[DEBUG_LOG] Token verification failed: Saved token mismatch") } else { - println("[DEBUG_LOG] Token verification successful: Token was correctly saved to the database") + println("[DEBUG_LOG] Token verification successful") } }app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (1)
36-44: Harden openUrlInBrowser: add scheme normalization, NEW_TASK for non-Activity, and safe resolveThis call can crash for invalid URLs or non-Activity contexts. Reuse the previously suggested robust variant.
- fun Context.openUrlInBrowser(url: String) { - val intent = Intent(Intent.ACTION_VIEW, url.toUri()) - startActivity(intent) - } + fun Context.openUrlInBrowser(url: String) { + val safe = url.trim() + if (safe.isEmpty()) return + val safeUri = (if ("://" in safe) safe else "https://$safe").toUri() + val intent = Intent(Intent.ACTION_VIEW, safeUri).apply { + if (this@openUrlInBrowser !is android.app.Activity) { + addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + } + } + try { + if (intent.resolveActivity(packageManager) != null) { + startActivity(intent) + } + } catch (_: Exception) { + // No-op: avoid crash if no handler is available + } + }
🧹 Nitpick comments (12)
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (1)
114-133: Logout success detection is brittle; confirm API contract and rely on structured fields or status codesSuccess is inferred by substring matches on
message. This is prone to regressions and localization issues. Prefer a boolean or status code the API guarantees, or extend the model to include one.If the backend can’t add a boolean, at least normalize and check a machine-readable field; otherwise, treat non-null
dataas definitive error and fall back to HTTP success.network/src/commonMain/kotlin/bose/ankush/network/auth/model/AuthModels.kt (1)
18-29: Verify wire names; consider @SerialName to align with backend JSONField names like
timestampOfRegistration,registrationSource, andfirebaseTokenassume camelCase on the wire. If the backend uses snake_case, add@SerialNameto prevent deserialization issues.Example for clarity:
// Example only — adjust to actual API @Serializable data class RegisterRequest( @SerialName("email") val email: String, @SerialName("password") val password: String, @SerialName("timestamp_of_registration") val timestampOfRegistration: String? = null, @SerialName("device_model") val deviceModel: String? = null, @SerialName("operating_system") val operatingSystem: String? = null, @SerialName("os_version") val osVersion: String? = null, @SerialName("app_version") val appVersion: String? = null, @SerialName("ip_address") val ipAddress: String? = null, @SerialName("registration_source") val registrationSource: String? = null, @SerialName("firebase_token") val firebaseToken: String? = null )app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt (1)
187-204: Prefer IPv4 and skip scoped/IPv6 addresses for getIpAddress()Current logic may return IPv6 or scoped addresses. Prefer IPv4 to match common backend expectations and avoid surprises.
- fun getIpAddress(): String? { + fun getIpAddress(): String? { try { val networkInterfaces = NetworkInterface.getNetworkInterfaces() while (networkInterfaces.hasMoreElements()) { val networkInterface = networkInterfaces.nextElement() val inetAddresses = networkInterface.inetAddresses while (inetAddresses.hasMoreElements()) { val inetAddress = inetAddresses.nextElement() - if (!inetAddress.isLoopbackAddress && !inetAddress.isLinkLocalAddress) { - return inetAddress.hostAddress - } + val isUsable = !inetAddress.isLoopbackAddress && !inetAddress.isLinkLocalAddress + val isIpv4 = inetAddress is java.net.Inet4Address + if (isUsable && isIpv4) return inetAddress.hostAddress } } } catch (_: Exception) { // Ignore exceptions and return null } return null }Add import:
import java.net.Inet4Addressapp/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (4)
158-161: Guard against empty queue when dismissing dialogs
removeAt(0)can throw if the queue is empty. Add a simple guard.- fun dismissDialog() { - permissionDialogQueue.removeAt(0) - } + fun dismissDialog() { + if (permissionDialogQueue.isNotEmpty()) { + permissionDialogQueue.removeAt(0) + } + }
345-355: Unify error mapping using errorResponseFromException for loginFor consistency with the rest of the ViewModel, prefer
errorResponseFromExceptionover rawDynamicText.fun login(email: String, password: String) { viewModelScope.launch { _authState.value = AuthState.Loading try { val response = authRepository.login(email, password) handleAuthResponse(response) } catch (e: Exception) { - _authState.value = AuthState.Error(UiText.DynamicText(e.message ?: "Login failed")) + _authState.value = AuthState.Error(errorResponseFromException(e)) } } }
362-395: Collect registration metadata off the main thread and unify error mapping
getIpAddress()and token fetch can be blocking. Move metadata collection + repository call to IO dispatcher; also useerrorResponseFromExceptionfor consistency.fun register(email: String, password: String) { viewModelScope.launch { _authState.value = AuthState.Loading try { - // Collect device information - val timestamp = Extension.getCurrentUtcTimestamp() - val deviceModel = Extension.getDeviceModel() - val operatingSystem = Extension.getOperatingSystem() - val osVersion = Extension.getOsVersion() - val appVersion = Extension.getAppVersion() - val ipAddress = Extension.getIpAddress() - val registrationSource = Extension.getRegistrationSource() - val firebaseToken = Extension.getFirebaseToken() - - // Call repository with enhanced data - val response = authRepository.register( - email = email, - password = password, - timestampOfRegistration = timestamp, - deviceModel = deviceModel, - operatingSystem = operatingSystem, - osVersion = osVersion, - appVersion = appVersion, - ipAddress = ipAddress, - registrationSource = registrationSource, - firebaseToken = firebaseToken - ) + val response = kotlinx.coroutines.withContext(dispatchers.io) { + // Collect device information off the main thread + val timestamp = Extension.getCurrentUtcTimestamp() + val deviceModel = Extension.getDeviceModel() + val operatingSystem = Extension.getOperatingSystem() + val osVersion = Extension.getOsVersion() + val appVersion = Extension.getAppVersion() + val ipAddress = Extension.getIpAddress() + val registrationSource = Extension.getRegistrationSource() + val firebaseToken = Extension.getFirebaseToken() + // Call repository with enhanced data + authRepository.register( + email = email, + password = password, + timestampOfRegistration = timestamp, + deviceModel = deviceModel, + operatingSystem = operatingSystem, + osVersion = osVersion, + appVersion = appVersion, + ipAddress = ipAddress, + registrationSource = registrationSource, + firebaseToken = firebaseToken + ) + } handleAuthResponse(response) } catch (e: Exception) { - _authState.value = - AuthState.Error(UiText.DynamicText(e.message ?: "Registration failed")) + _authState.value = AuthState.Error(errorResponseFromException(e)) } } }Add import if needed:
import kotlinx.coroutines.withContext
414-416: Unify error mapping for logout exceptionsUse the centralized mapper for consistency.
- } catch (e: Exception) { - _authState.value = AuthState.Error(UiText.DynamicText(e.message ?: "Logout failed")) - } + } catch (e: Exception) { + _authState.value = AuthState.Error(errorResponseFromException(e)) + }buildSrc/src/main/java/Versions.kt (2)
8-8: Compose BOM 2025.05.01 — check downstream AndroidX alignment (Navigation, Lifecycle, Accompanist)Some AndroidX libs here are relatively old (e.g., navigation=2.7.0, lifecycle=2.6.1, accompanist=0.28.0). While BOM governs Compose artifacts, these libs still bind to Compose runtime APIs. Consider bumping to versions contemporary with this BOM to reduce binary mismatch risk.
If desired, I can propose a minimal set of version bumps verified against the current BOM/AGP matrix.
54-55: Firebase BOM 32.2.0 is quite old relative to 2025 toolchainYou’ve just added Firebase Messaging downstream. Consider updating the BOM so analytics/perf/messaging align with modern Play services and Android 14/15 SDKs.
If you want, I can generate a PR-side bump with release notes references once you confirm the target minimum Android/Play Services versions.
app/build.gradle.kts (3)
78-82: StrongSkipping disabled — double-check necessityDisabling StrongSkipping can reduce recomposition performance and can mask certain stability guarantees. If you disabled it due to a specific bug, consider documenting the issue/ticket and adding a TODO with removal criteria. Otherwise, prefer defaults.
I can run a quick pass to locate any Compose state misuse that might have motivated this setting.
39-41: Packaging exclude for previous-compilation-data.bin — verify if still neededThis exclude was a common workaround. With newer Kotlin/Compose toolchains it may no longer be necessary. Keeping it is harmless, but consider removing if not needed to avoid masking real conflicts.
Try a clean build without this exclude; if no conflicts arise, drop it.
139-140: Firebase Messaging wired — ensure proguard/r8 rules if minify enabledRelease has minify and shrinkResources enabled. Firebase’s BOM usually adds keep rules via aapt2/consumer rules, but if you add messaging service receivers or custom serializers, ensure rules exist.
If you want, I can propose targeted -keep rules for common FCM setups (FirebaseMessagingService, WorkManager handlers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
app/build.gradle.kts(4 hunks)app/src/main/java/bose/ankush/weatherify/base/common/Extension.kt(4 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt(7 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt(9 hunks)buildSrc/src/main/java/Dependencies.kt(1 hunks)buildSrc/src/main/java/Versions.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/model/AuthModels.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepository.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt
- network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepository.kt
🧰 Additional context used
🧬 Code graph analysis (3)
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (1)
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (1)
errorResponseFromException(50-55)
app/build.gradle.kts (1)
buildSrc/src/main/kotlin/KmmDeps.kt (1)
implementation(56-58)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (4)
sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt (1)
rememberGlassmorphicSnackbarState(109-131)app/src/main/java/bose/ankush/weatherify/presentation/navigation/AppNavigation.kt (1)
AppNavigation(25-146)sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt (1)
LoginScreen(61-414)app/src/main/java/bose/ankush/weatherify/base/permissions/PermissionAlertDialog.kt (1)
PermissionAlertDialog(8-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
buildSrc/src/main/java/Versions.kt (4)
5-5: Kotlin 2.1.21 bump looks good; ensure AGP/Compose plugin alignmentGiven you’re using org.jetbrains.kotlin.plugin.compose, the Compose compiler is tied to Kotlin. This is fine, but it tightens the matrix with AGP and the Compose BOM. Make sure the CI matrix includes a clean sync/build on both debug and release to catch any toolchain drift.
Run a quick sanity build with --stacktrace to catch plugin mismatches early.
60-62: Hilt/Hilt-Compose bumps look consistenthilt=2.56.2 and hiltNavigationCompose=1.2.0 pair well with newer Kotlin/Compose setups. No concerns here.
17-21: Ktlint/Detekt split is a good moveSeparating Gradle plugin and CLI versions helps avoid resolution confusion. Make sure the root build uses these constants consistently for both CI formatting and local pre-commit hooks.
If you’d like, I can add a simple ./gradlew ktlintCheck detekt task wiring in the root for CI.
51-52: No action needed — Gson 2.13.1 is published on Maven Central
Fetched the Maven metadata from repo1.maven.org and confirmed that version 2.13.1 appears among the available releases (alongside 2.13.0, 2.12.1, etc.).buildSrc/src/main/java/Dependencies.kt (2)
73-76: Lottie removal aligns with Versions cleanupDeps.lottieCompose removal plus the absent Versions.lottie keeps things tidy. No issues.
61-62: FCM setup verified — all checks passI’ve confirmed that all required pieces for Firebase Cloud Messaging are in place:
- BOM import present in app/build.gradle.kts to govern all Firebase dependencies
<uses-permission android:name="android.permission.POST_NOTIFICATIONS"/>declared in app/src/main/AndroidManifest.xml- Notification channel creation implemented in WeatherifyApplication.kt (uses NotificationChannel and creates the channel at startup)
id("com.google.gms.google-services")applied in app/build.gradle.kts and google-services.json included in the app moduleNo further action needed here.
app/build.gradle.kts (3)
151-154: Lottie removal acknowledgedComment and removal look clean; coil-compose remains for image loading.
65-69: ExperimentalAnimationApi opt-in is still requiredWe’ve located direct uses of
@ExperimentalAnimationApiin the code, so the compiler opt-in should be retained:
- app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt: import at line 12, annotation at line 55
- app/src/main/java/bose/ankush/weatherify/presentation/navigation/AppNavigation.kt: import at line 5, annotation at line 26
No further changes needed here—please keep the
-Xopt-in=androidx.compose.animation.ExperimentalAnimationApiflag.
26-28: Use BCP-47 “he” instead of legacy “iw” (and verify DSL placement)The
localeFilterslist should use the modern Hebrew code “he” instead of the legacy “iw”:androidResources { - localeFilters.addAll(listOf("en", "hi", "iw")) + localeFilters.addAll(listOf("en", "hi", "he")) }Next steps:
- Please confirm which Android Gradle Plugin (AGP) version you’re on (e.g. check
gradle/libs.versions.tomlor the rootbuild.gradle.ktsfor the AGP version alias).- If you encounter a Gradle sync error about
androidResourcesplacement, move this block out of thedefaultConfig { … }scope into the top-levelandroid { androidResources { … } }block. I can then provide the exact diff for that relocation.
This commit introduces several improvements to authentication handling, logging, and dependency management:
- **Authentication Resilience:**
- `AuthInterceptor`: When a token refresh fails after a 401 response, the system now forces a logout by clearing the token via `TokenManager.forceLogout()` and emits a global `AuthEvent.Unauthorized` event. This event carries a user-friendly message for display.
- `TokenManager`: Added a `forceLogout()` method to clear the stored authentication token.
- `MainActivity`: Now observes `AuthEventBus` for `AuthEvent.Unauthorized` events. Upon receiving such an event, it displays a snackbar message to inform the user about the need to log in again for security reasons.
- **HTTP Client Configuration:**
- `IOSHttpClient` & `AndroidHttpClient`: Standardized JSON content negotiation. Removed redundant mixed content type registration (e.g., "application/json, text/html"). Specific content types should be handled per request if necessary.
- **Logging Improvements:**
- `AuthRepositoryImpl`: Condensed token verification log messages for brevity.
- `LocationService`: Replaced `println(exception.printStackTrace())` with `Timber.e` for structured error logging.
- `CurrentWeatherReportLayout`: Replaced `e.printStackTrace()` with `Timber.e` for structured error logging during geocoding failures.
- **Dependency Update:**
- `Versions.kt`: Updated Razorpay dependency from version `1.6.30` to `1.6.40`.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (2)
251-253: Remove auto-launch of location permissions to avoid double prompts.RequestLocationPermission currently auto-launches the permission request in LaunchedEffect(Unit) and also launches on the dialog confirm path. This can trigger overlapping/duplicate permission flows. Remove the auto-launch and let the dialog or an explicit event drive it.
- LaunchedEffect(key1 = Unit) { - locationPermissionsResultLauncher.launch(PERMISSIONS_TO_REQUEST) - }
258-276: Reset the notification‐permission launch flag after permission resultThe
onResultcallback forRequestPermissioncurrently only handles the granted case and never resets thelaunchNotificationPermissionflag. SinceupdateNotificationPermission(launchState = true)is only called inAppNavigation.kt, failing to set it back tofalsemeans the permission dialog can reappear on every recomposition or when re-entering the screen.Affected locations:
- MainActivity.kt (lines 258–276):
rememberLauncherForActivityResult→onResultblock needs the reset- MainViewModel.kt:
updateNotificationPermission(launchState: Boolean)is defined but never invoked withfalse- AppNavigation.kt: sets
launchState = trueon navigation action, without a corresponding resetProposed diff in
MainActivity.kt:val notificationPermissionResultLauncher = rememberLauncherForActivityResult( contract = ActivityResultContracts.RequestPermission(), onResult = { isGranted -> if (isGranted) { Toast.makeText( context, "Notification permission granted", Toast.LENGTH_SHORT ).show() // hide notification banner on settings screen viewModel.updateShowNotificationBannerState(false) } + // Always reset launch flag after the permission flow completes + viewModel.updateNotificationPermission(launchState = false) } )Verification output for
updateNotificationPermissionusages:MainViewModel.kt: fun updateNotificationPermission(launchState: Boolean) { … } AppNavigation.kt: viewModel.updateNotificationPermission(launchState = true)No existing call sets it back to
false, so this addition is required to prevent repeated permission prompts.
♻️ Duplicate comments (8)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (3)
153-168: Move side-effects out of composition; gate location fetch by permission via LaunchedEffect.Directly calling viewModel.fetchAndSaveLocationCoordinates() in composition risks firing on recompositions and before permission is actually granted. Wrap it in LaunchedEffect keyed by the current permission state and only render the permission UI when needed. This also addresses a previously raised comment.
isLoggedIn -> { // User is logged in, show main app content - val launchNotificationPermissionState = - viewModel.launchNotificationPermission.collectAsState() - if (locationClient.hasLocationPermission()) { - // if permission granted already then fetch and save location coordinates - viewModel.fetchAndSaveLocationCoordinates() - } else { - // request location permission - RequestLocationPermission(context) - } - if (launchNotificationPermissionState.value) { - // request notification permission - RequestNotificationPermission(context) - } + val launchNotificationPermissionState = + viewModel.launchNotificationPermission.collectAsState() + + val hasLocationPermission = locationClient.hasLocationPermission() + LaunchedEffect(hasLocationPermission) { + if (hasLocationPermission) { + viewModel.fetchAndSaveLocationCoordinates() + } + } + if (!hasLocationPermission) { + // request location permission + RequestLocationPermission(context) + } + if (launchNotificationPermissionState.value) { + // request notification permission + RequestNotificationPermission(context) + }
173-179: Key LaunchedEffect on the value, not the State object.Keying on the State instance prevents the effect from re-running when the underlying value changes. Use launchNotificationPermissionState.value. This was noted earlier and remains applicable.
- LaunchedEffect(key1 = launchNotificationPermissionState) { + LaunchedEffect(key1 = launchNotificationPermissionState.value) { if (!context.hasNotificationPermission()) { viewModel.updateShowNotificationBannerState(true) } else { viewModel.updateShowNotificationBannerState(false) } }
242-242: Fix permanently-declined logic for permission rationale.isPermanentlyDeclined should be true when the system will NOT show the rationale (i.e., !shouldShowRequestPermissionRationale(permission)). Current logic is inverted and will misroute users to Settings at the wrong time. This was flagged previously and still needs correction.
- isPermanentlyDeclined = shouldShowRequestPermissionRationale(permission), + isPermanentlyDeclined = !shouldShowRequestPermissionRationale(permission),network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (1)
39-46: Gate debug prints; avoid auth-related logs in productionGood job removing raw token values from logs (resolves the previous secret-leak concern). Keep going: guard the remaining auth debug prints behind a flag so they never emit in production builds.
Apply this small diff to add a flag and guard the prints:
@@ class AuthRepositoryImpl( ) : AuthRepository { + private val debugLogs = false @@ - if (savedToken != originalToken) { - println("[DEBUG_LOG] Token verification failed: token mismatch") - } else { - println("[DEBUG_LOG] Token verification successful") - } + if (debugLogs) { + if (savedToken != originalToken) { + println("[DEBUG_LOG] Token verification failed: token mismatch") + } else { + println("[DEBUG_LOG] Token verification successful") + } + }If you already have a multiplatform logger (Napier/Kermit), prefer that over
printlnand wire log enablement via DI or build config.Also applies to: 14-17
buildSrc/src/main/java/Versions.kt (1)
5-5: Kotlin 2.1.21 requires kotlinx-coroutines ≥ 1.8.0 — current 1.6.4 will break (update core + test).Keeping coroutines at 1.6.4 with Kotlin 2.1.21 risks compilation/link errors and runtime crashes. Please bump both the runtime and test artifacts to a Kotlin 2.1.x–compatible release (e.g., 1.10.2 or newer).
Apply this diff:
- const val coroutineTest = "1.7.1" + // Keep test lib aligned with kotlinx-coroutines + const val coroutineTest = "1.10.2" @@ - const val coroutines = "1.6.4" + // Kotlin 2.1.x requires kotlinx-coroutines >= 1.8.0 + const val coroutines = "1.10.2"Verification script:
#!/bin/bash set -euo pipefail echo "Repo-wide checks for coroutines usage and Compose plugin" echo "--------------------------------------------------------" rg -nP --glob '!**/build/**' '\bkotlinx-coroutines(-test)?\b' -C2 || true rg -nP --glob '!**/build/**' 'plugins\s*\{[^}]*org\.jetbrains\.kotlin\.plugin\.compose' -C2 || true echo "Artifact existence checks (AGP, Coroutines, Gson, Compose BOM)" echo "--------------------------------------------------------------" for url in \ 'https://dl.google.com/android/maven2/com/android/tools/build/gradle/maven-metadata.xml' \ 'https://repo1.maven.org/maven2/org/jetbrains/kotlinx/kotlinx-coroutines-core/maven-metadata.xml' \ 'https://repo1.maven.org/maven2/com/google/code/gson/gson/maven-metadata.xml' \ 'https://dl.google.com/android/maven2/androidx/compose/compose-bom/maven-metadata.xml' do echo "Latest for $(basename "$(dirname "$url")"):" curl -s "$url" | grep -Po '(?<=<latest>)[^<]+' | head -n1 || echo "N/A" donenetwork/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt (3)
31-37: Remove no-op validator.
HttpResponseValidatorthat just rethrows adds overhead without value.
44-44: Do not log full URLs; redact query parameters to avoid leaking tokens or PII.Log only scheme/host/path or strip everything after “?”. The current line may print sensitive query params.
-println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}") +val u = response.request.url +println("[DEBUG_LOG] 401 Unauthorized: ${u.protocol.name}://${u.host}${u.encodedPath}")
79-88: Legacy path: avoid runBlocking; mirror the suspendable interceptor.Make the legacy variant KMP-safe and consistent by using
HttpSendinstead ofdefaultRequest+runBlocking.fun HttpClientConfig<*>.configureAuth(tokenStorage: TokenStorage) { - defaultRequest { - runBlocking { - val token = tokenStorage.getToken() - if (!token.isNullOrBlank()) { - header("Authorization", "Bearer $token") - } - } - } + install(HttpSend) { + intercept { request -> + tokenStorage.getToken()?.takeIf { it.isNotBlank() }?.let { token -> + request.headers.remove(HttpHeaders.Authorization) + request.headers.append(HttpHeaders.Authorization, "Bearer $token") + } + execute(request) + } + } }
🧹 Nitpick comments (26)
network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt (2)
24-25: Verify backend Content-Type; potential regression if JSON is mislabeled.
json(json)only registers a converter forapplication/json. If your backend ever returns JSON withtext/htmlortext/plain(a not-uncommon quirk), deserialization will fail. Please confirm upstream responses (and integration tests) consistently useapplication/json; charset=utf-8. If not, consider adding guarded fallbacks:- // Register standard JSON handling; handle other content types per request if required. - json(json) + // Register standard JSON handling + json(json) + // Optional: guarded fallbacks for backends that mislabel JSON (enable via a feature flag/config) + // json(json, contentType = ContentType.Text.Plain) + // json(json, contentType = ContentType.Text.Html)Note: you'll need
import io.ktor.http.ContentTypeat the top if you enable the optional lines.
24-25: Nit: consider setting a default Accept header for GETs.Some servers negotiate responses based on
Accept. Adding a default can improve interoperability without loosening content-type matching:Kotlin (outside this block, in the same client config):
install(io.ktor.client.plugins.defaultrequest.DefaultRequest) { headers.append(io.ktor.http.HttpHeaders.Accept, io.ktor.http.ContentType.Application.Json.toString()) }app/src/main/java/bose/ankush/weatherify/base/location/LocationService.kt (4)
53-66: Flow catch placement only handles upstream; move after onEach to also catch collector/notify errors.With catch before onEach, exceptions thrown inside onEach (e.g., from NotificationManager.notify) won’t be caught and will cancel the coroutine. Placing catch after onEach covers both upstream and collector exceptions.
Apply:
locationClient .getLocationUpdates(interval = 1000L) - .catch { e -> Timber.e(e, "Location updates error") } - .onEach { location -> + .onEach { location -> val lat = location.latitude.toString().take(4) val long = location.longitude.toString().take(4) val updatedNotification = notification.setContentText("Location: $lat, $long") notificationManager.notify( NOTIFICATION_ID, updatedNotification.build() ) } + .catch { e -> Timber.e(e, "Location updates error") } .launchIn(serviceScope)
40-52: Create NotificationChannel before startForeground/notify on Android O+.If the channel isn’t created, notifications may be dropped and FGS start can fail on API 26+. If creation is handled elsewhere, ignore; otherwise, add this here.
Apply inside start(), right after obtaining notificationManager:
val notificationManager = getSystemService(NotificationManager::class.java) + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) { + val channel = android.app.NotificationChannel( + NOTIFICATION_CHANNEL_ID, + NOTIFICATION_NAME, + NotificationManager.IMPORTANCE_LOW + ).apply { + setShowBadge(false) + description = "Location updates" + } + notificationManager.createNotificationChannel(channel) + }Optionally add imports (if you prefer not to inline FQNs):
import android.app.NotificationChannel import android.os.Build
57-59: Deterministic coordinate formatting.toString().take(4) can yield odd results (e.g., “-122”). Format to fixed decimals for UX.
Apply:
- val lat = location.latitude.toString().take(4) - val long = location.longitude.toString().take(4) + val lat = String.format(java.util.Locale.US, "%.4f", location.latitude) + val long = String.format(java.util.Locale.US, "%.4f", location.longitude)
74-77: Proactively cancel the scope in stop() to halt updates immediately.stopSelf() leads to onDestroy eventually, but cancelling early prevents any in-flight emissions.
Apply:
private fun stop() { + serviceScope.cancel() stopForeground(STOP_FOREGROUND_REMOVE) stopSelf() }Note: If you ever re-start the same Service instance without recreating it, you’d need to reinitialize serviceScope in start(); not an issue if the system recreates the Service on next start.
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (3)
105-112: Externalize user-facing strings for localization."Authentication successful" and the Unauthorized fallback message should come from string resources to support localization and consistency.
- showSnackbar("Authentication successful") + showSnackbar(getString(R.string.auth_success)) ... - "You need to log in again to continue using the app for security purposes." + getString(R.string.auth_relogin_required)Also applies to: 126-130
78-87: Optional: Migrate away from Accompanist System UI Controller.If you’re on recent Activity/Compose versions, consider using Activity.enableEdgeToEdge and Material3 window insets APIs instead of accompanist-systemuicontroller, which has been gradually deprecated in favor of AndroidX equivalents. Current code works; this is a forward-looking refactor.
194-201: Nit: Consider centralizing policy URLs.Move the Terms/Privacy URLs into constants or string resources for easier maintenance and localization.
app/src/main/java/bose/ankush/weatherify/presentation/home/component/CurrentWeatherReportLayout.kt (5)
171-174: Tag the log and keep production noise low.Minor polish: add a tag so logs are easily searchable and consider harmonizing the message with other location logs.
Apply this diff:
- Timber.e(e, "Geocoding failed; using default location label") + Timber.tag("Geocoder").e(e, "Reverse geocoding failed; defaulting to location label")
156-165: Avoid “City, null” in the UI; join non-null parts safely.Slight tweak to ensure we never render “null” when country is missing.
Apply this diff:
- if (!addresses.isNullOrEmpty()) { - val address = addresses.firstOrNull() - val cityName = address?.locality ?: address?.subAdminArea - val countryName = address?.countryName - - when { - cityName != null -> "$cityName, $countryName" - else -> countryName ?: "Current Location" - } - } else { - "Current Location" - } + addresses?.firstOrNull()?.let { address -> + listOfNotNull( + address.locality ?: address.subAdminArea, + address.countryName + ).joinToString(", ") + } ?: "Current Location"
109-113: Localize hardcoded strings (“Today’s Forecast”, “Feels like…”, “Sunrise”, “Sunset”).Move literals to string resources and use formatting to aid i18n and accessibility.
Apply these diffs:
- Text( - text = "Today's Forecast", + Text( + text = stringResource(id = R.string.todays_forecast_title), style = MaterialTheme.typography.titleMedium,- Text( - text = "Feels like ${currentWeather.feels_like?.toCelsius()}°", + val feelsLikeText = currentWeather.feels_like?.toCelsius()?.let { + stringResource(id = R.string.degree, it) + } ?: stringResource(id = R.string.not_available) + Text( + text = stringResource(id = R.string.feels_like, feelsLikeText), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onBackground.copy(alpha = 0.7f) )- Text( - text = "Sunrise", + Text( + text = stringResource(id = R.string.sunrise_label), style = MaterialTheme.typography.bodySmall,- Text( - text = "Sunset", + Text( + text = stringResource(id = R.string.sunset_label), style = MaterialTheme.typography.bodySmall,Then add to
res/values/strings.xml:<resources> <string name="todays_forecast_title">Today’s forecast</string> <string name="feels_like">Feels like %1$s</string> <string name="sunrise_label">Sunrise</string> <string name="sunset_label">Sunset</string> </resources>Also applies to: 237-241, 395-398, 425-428
443-459: Respect 24‑hour settings and locale; don’t force “AM/PM”.Forcing AM/PM breaks 24‑hour locales and can be wrong seasonally/latitudinally. Format based on user settings and locale instead.
Apply this diff:
@Composable private fun formatTimeWithAmPm(timestamp: Int?, isSunrise: Boolean): String { - if (timestamp == null) return "N/A" - - // Use remember to cache the formatted time based on the timestamp and isSunrise flag - return remember(timestamp, isSunrise) { - val date = Date(timestamp.toLong() * 1000) - val timeWithoutAmPm = hourMinuteFormatter.format(date) - - // Force AM for sunrise, PM for sunset - if (isSunrise) { - "$timeWithoutAmPm AM" - } else { - "$timeWithoutAmPm PM" - } - } + if (timestamp == null) return stringResource(id = R.string.not_available) + val context = LocalContext.current + // Cache by timestamp; format pattern does not depend on isSunrise + return remember(timestamp) { + val date = Date(timestamp.toLong() * 1000) + val pattern = if (android.text.format.DateFormat.is24HourFormat(context)) { + "HH:mm" + } else { + "h:mm a" + } + SimpleDateFormat(pattern, Locale.getDefault()).format(date) + } }Optional follow-up: remove or repurpose
hourMinuteFormatterif no longer used.
146-171: Leverage the modern GeocodeListener API for reverse geocoding on Android 13+Our project’s
compileSdkVersionandtargetSdkVersionare both set to 36 (inbuildSrc/src/main/java/ConfigData.kt), so you can safely reference the non-blockingGeocodeListeneravailable in API 33+ and fall back on the deprecated synchronous call for older devices.• In CurrentWeatherReportLayout.kt (lines 146–171), replace the blocking
Geocoder.getFromLocation(...)call with a coroutine-friendlyGeocodeListenerwhenBuild.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU, retaining the old path for older APIs.
• Add the necessary imports for coroutine support andBuildchecks.Apply this diff:
--- a/app/src/main/java/bose/ankush/weatherify/presentation/home/component/CurrentWeatherReportLayout.kt +++ b/app/src/main/java/bose/ankush/weatherify/presentation/home/component/CurrentWeatherReportLayout.kt @@ -146,31 +146,49 @@ try { - val result = withContext(Dispatchers.IO) { - val geocoder = Geocoder(context, Locale.getDefault()) - - @Suppress("DEPRECATION") - val addresses = geocoder.getFromLocation( - userLocation.first, - userLocation.second, - 1 - ) - - if (!addresses.isNullOrEmpty()) { - val address = addresses.firstOrNull() - val cityName = address?.locality ?: address?.subAdminArea - val countryName = address?.countryName - - when { - cityName != null -> "$cityName, $countryName" - else -> countryName ?: "Current Location" - } - } else { - "Current Location" - } - } - // Update state only once after background processing is complete - locationName = result + locationName = withContext(Dispatchers.IO) { + if (!Geocoder.isPresent()) return@withContext "Current Location" + val geocoder = Geocoder(context, Locale.getDefault()) + + val addresses: List<android.location.Address> = + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + suspendCancellableCoroutine { cont -> + geocoder.getFromLocation( + userLocation.first, + userLocation.second, + 1, + object : Geocoder.GeocodeListener { + override fun onGeocode(results: MutableList<android.location.Address>) { + if (cont.isActive) cont.resume(results) + } + override fun onError(errorMessage: String?) { + if (cont.isActive) cont.resume(emptyList()) + } + } + ) + } + } else { + @Suppress("DEPRECATION") + geocoder.getFromLocation( + userLocation.first, + userLocation.second, + 1 + ).orEmpty() + } + + addresses.firstOrNull()?.let { address -> + listOfNotNull( + address.locality ?: address.subAdminArea, + address.countryName + ).joinToString(", ") + } ?: "Current Location" + } } catch (e: Exception) { // existing error handling… }And add these at the top of the file:
import android.os.Build import kotlin.coroutines.resume import kotlinx.coroutines.suspendCancellableCoroutinenetwork/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (6)
23-31: DRY up token save + verification across login/register/refreshThe token persistence/verification pattern is duplicated in three places. Centralize it to reduce risk of inconsistent changes and make it easier to add telemetry/error handling later.
Apply these diffs:
@@ - // Save token on successful login - val token = response.data?.token - if (response.isSuccess() && token != null && token.isNotBlank()) { - tokenStorage.saveToken(token) - - // Verify token was saved correctly - verifyTokenSaved(token) - } + saveAndVerifyTokenFrom(response) @@ - // Save token on successful registration - val token = response.data?.token - if (response.isSuccess() && token != null && token.isNotBlank()) { - tokenStorage.saveToken(token) - - // Verify token was saved correctly - verifyTokenSaved(token) - } + saveAndVerifyTokenFrom(response) @@ - // Save new token on successful refresh - val token = response.data?.token - if (response.isSuccess() && token != null && token.isNotBlank()) { - tokenStorage.saveToken(token) - - // Verify token was saved correctly - verifyTokenSaved(token) - } + saveAndVerifyTokenFrom(response)Add this helper (place after
verifyTokenSaved):@@ private suspend fun verifyTokenSaved(originalToken: String) { val savedToken = tokenStorage.getToken() if (savedToken != originalToken) { - println("[DEBUG_LOG] Token verification failed: token mismatch") + if (debugLogs) println("[DEBUG_LOG] Token verification failed: token mismatch") } else { - println("[DEBUG_LOG] Token verification successful") + if (debugLogs) println("[DEBUG_LOG] Token verification successful") } } + + private suspend fun saveAndVerifyTokenFrom(response: AuthResponse) { + val token = response.data?.token + if (response.isSuccess() && !token.isNullOrBlank()) { + tokenStorage.saveToken(token) + verifyTokenSaved(token) + } + }Also applies to: 74-82, 100-107
94-110: Consider invalid refresh handling (401/invalid token)If
refreshToken()fails due to invalid/expired token, you likely want to clear the stored token and surface an “auth required” state. Right now, the stored token is left intact on refresh failure.If
AuthResponseexposes a status code or error code, you could handle it like:val response = apiService.refreshToken(request) if (!response.isSuccess() && response.code == 401 /* or errorCode == "INVALID_TOKEN" */) { tokenStorage.clearToken() }Confirm the error contract of
AuthApiService.refreshTokenand whether 401/403 or a specific domain error is returned on invalid refresh.
19-33: Unify error handling strategy across auth calls
login,register, andrefreshTokenlet exceptions bubble up, whilelogoutwraps results inResult. Aligning to a single strategy makes call sites simpler. Either:
- Wrap all API calls in
Resultand map to domain errors, or- Ensure
AuthApiServicenever throws (returnsAuthResponseerrors instead).Would you like a follow-up patch that standardizes on one approach?
Also applies to: 48-84, 94-110
35-46: Verification helper is fine, but prefer test-time checks over runtime I/OReading the token back after every save is extra I/O in hot paths. Consider:
- Keep this helper only in debug builds, or
- Remove it and add unit tests that assert
saveTokenstores what is expected.I can provide a small FakeTokenStorage and tests to validate save/refresh flows without runtime logging.
112-131: Nit: variable shadowing ofmessageWithin the
elsebranch you redeclaremessage, shadowing the outermessage. If you keep the current structure, rename the inner variable to avoid confusion.If you adopt the logout refactor above, this issue disappears.
19-110: Add unit tests for auth flowsRecommend tests (with fakes/mocks) to pin behavior:
- login: saves token on success; does not save on failure.
- register: same as login.
- refreshToken: returns null when no token; saves new token on success; clears token on 401/invalid (if you implement that).
- logout: clears token locally even when network fails.
I can scaffold these tests with a
FakeTokenStorageand a stubbedAuthApiService. Want me to open an issue and attach a patch?buildSrc/src/main/java/Versions.kt (1)
17-20: Good split of KtLint plugin vs CLI; add usage notes to avoid dual-run confusion.Separating plugin and CLI avoids resolution collisions. To prevent redundant runs, document in build logic which one is the source of truth (Gradle task vs CI CLI call). Optional: add a comment with the intended execution path.
Suggested inline note:
// KtLint versions: separate plugin and CLI to avoid resolution confusion + // Policy: Prefer Gradle tasks locally (ktlintCheck/Format) and run the CLI only in CI for pre-commit checks. const val ktLintGradlePlugin = "12.1.1" const val ktLintCli = "1.3.1" const val detekt = "1.23.6"network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt (5)
14-19: Fix KDoc: function doesn’t return an HttpClient instance.The extension mutates config in place and returns Unit; the
@returndescription is misleading./** * Helper function to configure a HttpClient with authentication * * @param tokenManager The manager for JWT tokens - * @return A configured HttpClient with authentication headers and token refresh + * Side effects: installs auth-related plugins on this HttpClientConfig (adds Authorization header, + * handles 401, optionally emits Unauthorized event). */
26-26: Prefer HttpHeaders.Authorization over string literal.Use the framework constant for accuracy and consistency.
- header("Authorization", "Bearer $token") + header(io.ktor.http.HttpHeaders.Authorization, "Bearer $token")
53-57: Don’t swallow exceptions on logout; at least log the error.Silent failures make diagnosis harder; you already log in the next catch.
- try { - // Clear token so that app considers user logged out - tokenManager.forceLogout() - } catch (_: Exception) { - } + try { + tokenManager.forceLogout() + } catch (e: Exception) { + println("[DEBUG_LOG] forceLogout failed: ${e.message}") + }
60-67: Consider moving UI-facing event emission out of the low-level network layer.Emitting global UI events (AuthEventBus) inside the interceptor couples networking and presentation; centralize in a higher layer or guard against duplicate emissions across concurrent 401s.
If keeping it here, ensure single emission per refresh failure burst. Verify
TokenManagercoordinates this (single-flight + debounce). If not, I can draft a smallUnauthorizedNotifierwith atomic gating.
20-73: Add tests with MockEngine: header attach, 401 refresh + retry, and event emission.Small integration tests would lock in behavior across platforms.
Example (Kotlin/MPP, illustrative):
class AuthInterceptorTest { @Test fun `adds Authorization header and retries once on 401 GET`() = runTest { val tokenManager = FakeTokenManager( initialToken = "expired", refreshedToken = "refreshed" ) var attempt = 0 val engine = MockEngine { req -> attempt++ if (attempt == 1) respond("", HttpStatusCode.Unauthorized) else respond("""{"ok":true}""", HttpStatusCode.OK) } val client = HttpClient(engine) { configureAuth(tokenManager) } val res = client.get("https://api.example.com/data") assertEquals(HttpStatusCode.OK, res.status) assertEquals(2, attempt) } }I can add these under
network/src/commonTest/...if you’d like.Also applies to: 79-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/src/main/java/bose/ankush/weatherify/base/location/LocationService.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt(7 hunks)app/src/main/java/bose/ankush/weatherify/presentation/home/component/CurrentWeatherReportLayout.kt(2 hunks)buildSrc/src/main/java/Versions.kt(2 hunks)network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt(1 hunks)network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt
- network/src/commonMain/kotlin/bose/ankush/network/auth/token/TokenManager.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (4)
sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt (1)
rememberGlassmorphicSnackbarState(109-131)app/src/main/java/bose/ankush/weatherify/presentation/navigation/AppNavigation.kt (1)
AppNavigation(25-146)sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt (1)
LoginScreen(61-414)app/src/main/java/bose/ankush/weatherify/base/permissions/PermissionAlertDialog.kt (1)
PermissionAlertDialog(8-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: qodana
🔇 Additional comments (8)
network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt (1)
24-25: LGTM: Standard JSON negotiation is simpler and consistent.Moving to
json(json)removes custom content-type wiring and aligns with typical Ktor usage and likely with the Android client. Public API remains unchanged.app/src/main/java/bose/ankush/weatherify/base/location/LocationService.kt (1)
17-17: Switch to Timber: aligned with project-wide logging.Good move replacing printStackTrace with Timber; consistent and structured logging.
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (1)
90-113: Snackbar integration and auth-state reset look solid.Good use of rememberGlassmorphicSnackbarState and resetting authState after surfacing messages to avoid repeated toasts/snackbars.
app/src/main/java/bose/ankush/weatherify/presentation/home/component/CurrentWeatherReportLayout.kt (1)
48-48: LGTM: Timber adoption here is appropriate.Adding Timber aligns with the broader logging improvements and keeps the UI flow unchanged.
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt (2)
14-17: Solid baseline: clear responsibilities and DI-friendly designConstructor wiring and class responsibility look good. Separation between API and storage layers is clear.
86-92: LGTM: simple, reactive login state signal
isLoggedIn()andgetToken()delegation toTokenStorageare straightforward and maintainable.buildSrc/src/main/java/Versions.kt (2)
60-61: Hilt setup looks solid – versions and KAPT blocks verifiedI confirmed that:
- Your
Versions.hiltComposevalue (1.2.0) lines up with theandroidx.hilt:hilt-navigation-composeconstant inbuildSrc/src/main/java/Dependencies.kt.- Both the
appandstoragemodules have akapt { … }block enabled for annotation processors (e.g. Room schema generation).Please double-check the following before we finalize:
- You’ve actually added the Navigation-Compose dependency where it’s needed, for example:
•implementation(Dependencies.hiltNavigationCompose)in your UI/presentation module.- Every Hilt-using module applies both the Kotlin KAPT plugin and the Hilt Gradle plugin, for example:
plugins { id("org.jetbrains.kotlin.android") id("kotlin-kapt") id("com.google.dagger.hilt.android") }- To suppress any “aggregating task” warnings from Hilt’s annotation processor, include the recommended Gradle option in your
kaptblock. For example:kapt { correctErrorTypes = true }Once those checks are in place, you should be all set.
71-71: Confirm Razorpay SDK availability and ProGuard integration• com.razorpay:checkout:1.6.40 is published to Maven Central (status 200) and even v1.6.41 is currently the latest release.
• The downloaded AAR (v1.6.40) does not include any embedded ProGuard/R8 consumer-rules files—be sure to copy the rules from Razorpay’s integration guide into yourproguard-rules.pro(or equivalent) to prevent runtime crashes in release builds.
• If you’re targeting Android 14/15, verify compatibility either by running end-to-end tests on those OS versions or by consulting Razorpay’s release notes for any restricted-API usage.
| val notificationManager = | ||
| getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | ||
| getSystemService(NOTIFICATION_SERVICE) as NotificationManager | ||
|
|
There was a problem hiding this comment.
NOTIFICATION_SERVICE is unresolved without import; prefer typed getSystemService.
Using NOTIFICATION_SERVICE unqualified will not compile unless you import android.content.Context.NOTIFICATION_SERVICE or prefix with Context.. The safer, modern API is the typed overload.
Apply:
- val notificationManager =
- getSystemService(NOTIFICATION_SERVICE) as NotificationManager
+ val notificationManager = getSystemService(NotificationManager::class.java)📝 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.
| val notificationManager = | |
| getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | |
| getSystemService(NOTIFICATION_SERVICE) as NotificationManager | |
| val notificationManager = getSystemService(NotificationManager::class.java) |
🤖 Prompt for AI Agents
In app/src/main/java/bose/ankush/weatherify/base/location/LocationService.kt
around lines 50 to 52, NOTIFICATION_SERVICE is used unqualified which won't
compile without importing android.content.Context.NOTIFICATION_SERVICE; replace
it with the typed API by calling
getSystemService(NotificationManager::class.java) (or alternatively qualify as
Context.NOTIFICATION_SERVICE and cast), and update imports accordingly so the
NotificationManager is obtained in a null-safe, modern way.
| import bose.ankush.network.auth.storage.TokenStorage | ||
| import bose.ankush.network.auth.token.TokenManager | ||
| import io.ktor.client.HttpClientConfig | ||
| import io.ktor.client.plugins.HttpResponseValidator | ||
| import io.ktor.client.plugins.defaultRequest | ||
| import io.ktor.client.plugins.observer.ResponseObserver | ||
| import io.ktor.client.request.header | ||
| import io.ktor.client.statement.request | ||
| import io.ktor.http.HttpStatusCode | ||
| import kotlinx.coroutines.runBlocking | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace blocking/unused imports; add suspend-friendly Ktor plugins.
Drop runBlocking, defaultRequest, ResponseObserver, and HttpResponseValidator imports. Add HttpSend and HttpHeaders for a suspendable, portable setup.
import bose.ankush.network.auth.storage.TokenStorage
import bose.ankush.network.auth.token.TokenManager
import io.ktor.client.HttpClientConfig
-import io.ktor.client.plugins.HttpResponseValidator
-import io.ktor.client.plugins.defaultRequest
-import io.ktor.client.plugins.observer.ResponseObserver
-import io.ktor.client.request.header
+import io.ktor.client.plugins.HttpSend
import io.ktor.client.statement.request
import io.ktor.http.HttpStatusCode
-import kotlinx.coroutines.runBlocking
+import io.ktor.http.HttpHeaders📝 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.
| import bose.ankush.network.auth.storage.TokenStorage | |
| import bose.ankush.network.auth.token.TokenManager | |
| import io.ktor.client.HttpClientConfig | |
| import io.ktor.client.plugins.HttpResponseValidator | |
| import io.ktor.client.plugins.defaultRequest | |
| import io.ktor.client.plugins.observer.ResponseObserver | |
| import io.ktor.client.request.header | |
| import io.ktor.client.statement.request | |
| import io.ktor.http.HttpStatusCode | |
| import kotlinx.coroutines.runBlocking | |
| import bose.ankush.network.auth.storage.TokenStorage | |
| import bose.ankush.network.auth.token.TokenManager | |
| import io.ktor.client.HttpClientConfig | |
| import io.ktor.client.plugins.HttpSend | |
| import io.ktor.client.statement.request | |
| import io.ktor.http.HttpStatusCode | |
| import io.ktor.http.HttpHeaders |
🤖 Prompt for AI Agents
In
network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt
around lines 3 to 13, remove the blocking and unused imports
(kotlinx.coroutines.runBlocking, io.ktor.client.plugins.defaultRequest,
io.ktor.client.plugins.observer.ResponseObserver,
io.ktor.client.plugins.HttpResponseValidator) and replace them with the
suspend-friendly Ktor plugin imports (io.ktor.client.plugins.HttpSend and
io.ktor.http.HttpHeaders); keep TokenStorage/TokenManager and any used
request/header APIs, and update any plugin usage in the file to use HttpSend and
HttpHeaders so the interceptor is suspendable and portable.
| // Add authorization header to all requests | ||
| defaultRequest { | ||
| runBlocking { | ||
| val token = tokenManager.getValidToken() | ||
| if (!token.isNullOrBlank()) { | ||
| header("Authorization", "Bearer $token") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Handle 401 Unauthorized responses by refreshing the token | ||
| HttpResponseValidator { | ||
| handleResponseExceptionWithRequest { exception, request -> | ||
| // Re-throw the exception to let the caller handle it | ||
| throw exception | ||
| } | ||
| } | ||
|
|
||
| // Add response observer to handle 401 responses | ||
| install(ResponseObserver) { | ||
| onResponse { response -> | ||
| if (response.status == HttpStatusCode.Unauthorized) { | ||
| // Log the 401 response | ||
| println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}") | ||
|
|
||
| // Attempt to refresh the token | ||
| runBlocking { | ||
| val refreshed = tokenManager.handleUnauthorized() | ||
| if (refreshed) { | ||
| println("[DEBUG_LOG] Token refreshed successfully after 401 response") | ||
| } else { | ||
| println("[DEBUG_LOG] Failed to refresh token after 401 response; forcing logout and notifying UI") | ||
| try { | ||
| // Clear token so that app considers user logged out | ||
| tokenManager.forceLogout() | ||
| } catch (_: Exception) { | ||
| } | ||
| // Emit a global unauthorized event for the UI to react (navigate to login + snackbar) | ||
| try { | ||
| bose.ankush.network.auth.events.AuthEventBus.emit( | ||
| bose.ankush.network.auth.events.AuthEvent.Unauthorized( | ||
| message = "For security, please log in again to continue using the app." | ||
| ) | ||
| ) | ||
| } catch (e: Exception) { | ||
| println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Eliminate runBlocking; implement suspendable token attach and one retry on 401 inside HttpSend.
Blocking in KMP risks deadlocks and UI freezes; the current 401 flow refreshes but never retries the failed call. Move to a suspend-friendly HttpSend interceptor that:
- Adds Authorization without blocking
- Redacts URL in logs
- Refreshes token on 401 and retries once (idempotent methods only) with the new token
- Falls back to logout + Unauthorized event if refresh fails
Run the script below to confirm Ktor version and whether getValidToken/handleUnauthorized are suspend:
Apply this diff to the function body:
fun HttpClientConfig<*>.configureAuth(tokenManager: TokenManager) {
- // Add authorization header to all requests
- defaultRequest {
- runBlocking {
- val token = tokenManager.getValidToken()
- if (!token.isNullOrBlank()) {
- header("Authorization", "Bearer $token")
- }
- }
- }
-
- // Handle 401 Unauthorized responses by refreshing the token
- HttpResponseValidator {
- handleResponseExceptionWithRequest { exception, request ->
- // Re-throw the exception to let the caller handle it
- throw exception
- }
- }
-
- // Add response observer to handle 401 responses
- install(ResponseObserver) {
- onResponse { response ->
- if (response.status == HttpStatusCode.Unauthorized) {
- // Log the 401 response
- println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}")
-
- // Attempt to refresh the token
- runBlocking {
- val refreshed = tokenManager.handleUnauthorized()
- if (refreshed) {
- println("[DEBUG_LOG] Token refreshed successfully after 401 response")
- } else {
- println("[DEBUG_LOG] Failed to refresh token after 401 response; forcing logout and notifying UI")
- try {
- // Clear token so that app considers user logged out
- tokenManager.forceLogout()
- } catch (_: Exception) {
- }
- // Emit a global unauthorized event for the UI to react (navigate to login + snackbar)
- try {
- bose.ankush.network.auth.events.AuthEventBus.emit(
- bose.ankush.network.auth.events.AuthEvent.Unauthorized(
- message = "For security, please log in again to continue using the app."
- )
- )
- } catch (e: Exception) {
- println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}")
- }
- }
- }
- }
- }
- }
+ install(HttpSend) {
+ intercept { request ->
+ // Attach token (non-blocking)
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { token ->
+ request.headers.remove(HttpHeaders.Authorization)
+ request.headers.append(HttpHeaders.Authorization, "Bearer $token")
+ }
+
+ var call = execute(request)
+ if (call.response.status == HttpStatusCode.Unauthorized) {
+ // Redacted URL (avoid leaking query params)
+ val url = call.request.url
+ println("[DEBUG_LOG] 401 Unauthorized: ${url.protocol.name}://${url.host}${url.encodedPath}")
+
+ val refreshed = tokenManager.handleUnauthorized()
+ if (refreshed) {
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { newToken ->
+ request.headers.remove(HttpHeaders.Authorization)
+ request.headers.append(HttpHeaders.Authorization, "Bearer $newToken")
+ }
+ // Retry once for idempotent methods only
+ val m = request.method.value
+ if (m == "GET" || m == "HEAD") {
+ call = execute(request)
+ }
+ } else {
+ // Logout + notify UI
+ try {
+ tokenManager.forceLogout()
+ } catch (e: Exception) {
+ println("[DEBUG_LOG] forceLogout failed: ${e.message}")
+ }
+ try {
+ bose.ankush.network.auth.events.AuthEventBus.emit(
+ bose.ankush.network.auth.events.AuthEvent.Unauthorized(
+ message = "For security, please log in again to continue using the app."
+ )
+ )
+ } catch (e: Exception) {
+ println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}")
+ }
+ }
+ }
+ call
+ }
+ }
}Notes:
- If
handleUnauthorized()is not suspend, consider usingHttpRequestRetryinstead. If it is suspend (likely), keeping all logic inHttpSendavoids plugin API mismatch. - Ensure
TokenManagersingle-flights refresh to prevent stampedes under concurrent 401s (e.g.,Mutexwith deferred result).
🏁 Script executed:
#!/bin/bash
# Ktor version
fd -H -a 'libs.versions.toml' -E '**/build/**' | xargs -I{} rg -n 'ktor' {}
rg -n 'io\.ktor' -g '!**/build/**' -C2
# TokenManager API (are these suspend?)
rg -nP -g '!**/build/**' -C2 'suspend\s+fun\s+(getValidToken|handleUnauthorized)\b'Length of output: 9453
Confirm Ktor Version via KmmVersions.kt
Please verify the exact Ktor client version used by inspecting KmmVersions.kt:
- Locate the file and print the
ktorconstant.
🏁 Script executed:
#!/usr/bin/env bash
# Locate KmmVersions.kt and print the Ktor version
FILE=$(fd --hidden --exclude build --exclude .gradle KmmVersions.kt)
echo "Found file: $FILE"
grep -n 'const val ktor' "$FILE"Length of output: 170
To pinpoint where the Ktor client version is specified, let’s locate any Versions file or references to KmmVersions in the build scripts:
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "🔍 Searching for any file referencing 'KmmVersions'..."
rg -l 'KmmVersions' -g '!**/build/**' || true
echo
echo "🗂️ If found, displaying the first match (first 50 lines):"
FILE=$(rg -l 'KmmVersions' -g '!**/build/**' | head -n1 || true)
if [[ -n "$FILE" && -f "$FILE" ]]; then
echo "=== $FILE ==="
sed -n '1,50p' "$FILE"
else
echo "No direct 'KmmVersions' references found."
fi
echo
echo "📑 Listing all Kotlin files with 'Versions' in the name:"
fd --type f --extension kt '.*Versions.*\.kt' -E '**/build/**'Length of output: 2895
Replace blocking logic with suspendable HttpSend interceptor (Ktor 2.3.13)
We’ve confirmed that you’re on Ktor 2.3.13 and both getValidToken and handleUnauthorized are suspend functions, so you can fully eliminate runBlocking here.
Key changes:
- Remove the existing
defaultRequest,HttpResponseValidator, andResponseObserverblocks that rely onrunBlocking. - Install the
HttpSendplugin to:- Asynchronously attach the latest token to every request.
- On a 401, redact query params in your log, call
handleUnauthorized(), and—if successful—retry idempotent methods (GET/HEAD) with the new token. - If refresh fails, invoke
tokenManager.forceLogout()and emit the global Unauthorized event.
- Ensure your
TokenManageruses a single-flight mechanism (e.g., aMutexwith a deferred result) to prevent concurrent refresh storms under multiple 401s.
Apply this diff in AuthInterceptor.kt:
fun HttpClientConfig<*>.configureAuth(tokenManager: TokenManager) {
- // Add authorization header to all requests
- defaultRequest {
- runBlocking {
- val token = tokenManager.getValidToken()
- if (!token.isNullOrBlank()) {
- header("Authorization", "Bearer $token")
- }
- }
- }
-
- // Handle 401 Unauthorized responses by refreshing the token
- HttpResponseValidator {
- handleResponseExceptionWithRequest { exception, request ->
- // Re-throw the exception to let the caller handle it
- throw exception
- }
- }
-
- // Add response observer to handle 401 responses
- install(ResponseObserver) {
- onResponse { response ->
- if (response.status == HttpStatusCode.Unauthorized) {
- // Log the 401 response
- println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}")
-
- // Attempt to refresh the token
- runBlocking {
- val refreshed = tokenManager.handleUnauthorized()
- if (refreshed) {
- println("[DEBUG_LOG] Token refreshed successfully after 401 response")
- } else {
- println("[DEBUG_LOG] Failed to refresh token after 401 response; forcing logout and notifying UI")
- try {
- // Clear token so that app considers user logged out
- tokenManager.forceLogout()
- } catch (_: Exception) {
- }
- // Emit a global unauthorized event for the UI to react (navigate to login + snackbar)
- try {
- bose.ankush.network.auth.events.AuthEventBus.emit(
- bose.ankush.network.auth.events.AuthEvent.Unauthorized(
- message = "For security, please log in again to continue using the app."
- )
- )
- } catch (e: Exception) {
- println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}")
- }
- }
- }
- }
- }
- }
+ install(HttpSend) {
+ intercept { request ->
+ // Attach token asynchronously
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { token ->
+ request.headers.remove(HttpHeaders.Authorization)
+ request.headers.append(HttpHeaders.Authorization, "Bearer $token")
+ }
+
+ var call = execute(request)
+ if (call.response.status == HttpStatusCode.Unauthorized) {
+ // Log only host and path to avoid leaking sensitive query params
+ val url = call.request.url
+ println("[DEBUG_LOG] 401 Unauthorized: ${url.protocol.name}://${url.host}${url.encodedPath}")
+
+ if (tokenManager.handleUnauthorized()) {
+ // Retry idempotent methods with refreshed token
+ tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { newToken ->
+ request.headers.remove(HttpHeaders.Authorization)
+ request.headers.append(HttpHeaders.Authorization, "Bearer $newToken")
+ }
+ if (request.method in listOf(HttpMethod.Get, HttpMethod.Head)) {
+ call = execute(request)
+ }
+ } else {
+ // Refresh failed → force logout + notify UI
+ runCatching { tokenManager.forceLogout() }
+ runCatching {
+ bose.ankush.network.auth.events.AuthEventBus.emit(
+ bose.ankush.network.auth.events.AuthEvent.Unauthorized(
+ message = "For security, please log in again to continue using the app."
+ )
+ )
+ }.onFailure { e ->
+ println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}")
+ }
+ }
+ }
+ call
+ }
+ }
}Mandatory points:
- Verify the single-flight (mutex) logic in
TokenManagerprevents concurrent refreshes. - Confirm Ktor 2.3.13’s
HttpSendplugin meets your needs (it does). - Remove all
runBlockingimports after refactoring.
📝 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.
| // Add authorization header to all requests | |
| defaultRequest { | |
| runBlocking { | |
| val token = tokenManager.getValidToken() | |
| if (!token.isNullOrBlank()) { | |
| header("Authorization", "Bearer $token") | |
| } | |
| } | |
| } | |
| // Handle 401 Unauthorized responses by refreshing the token | |
| HttpResponseValidator { | |
| handleResponseExceptionWithRequest { exception, request -> | |
| // Re-throw the exception to let the caller handle it | |
| throw exception | |
| } | |
| } | |
| // Add response observer to handle 401 responses | |
| install(ResponseObserver) { | |
| onResponse { response -> | |
| if (response.status == HttpStatusCode.Unauthorized) { | |
| // Log the 401 response | |
| println("[DEBUG_LOG] Received 401 Unauthorized response from ${response.request.url}") | |
| // Attempt to refresh the token | |
| runBlocking { | |
| val refreshed = tokenManager.handleUnauthorized() | |
| if (refreshed) { | |
| println("[DEBUG_LOG] Token refreshed successfully after 401 response") | |
| } else { | |
| println("[DEBUG_LOG] Failed to refresh token after 401 response; forcing logout and notifying UI") | |
| try { | |
| // Clear token so that app considers user logged out | |
| tokenManager.forceLogout() | |
| } catch (_: Exception) { | |
| } | |
| // Emit a global unauthorized event for the UI to react (navigate to login + snackbar) | |
| try { | |
| bose.ankush.network.auth.events.AuthEventBus.emit( | |
| bose.ankush.network.auth.events.AuthEvent.Unauthorized( | |
| message = "For security, please log in again to continue using the app." | |
| ) | |
| ) | |
| } catch (e: Exception) { | |
| println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}") | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| fun HttpClientConfig<*>.configureAuth(tokenManager: TokenManager) { | |
| install(HttpSend) { | |
| intercept { request -> | |
| // Attach token asynchronously | |
| tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { token -> | |
| request.headers.remove(HttpHeaders.Authorization) | |
| request.headers.append(HttpHeaders.Authorization, "Bearer $token") | |
| } | |
| var call = execute(request) | |
| if (call.response.status == HttpStatusCode.Unauthorized) { | |
| // Log only host and path to avoid leaking sensitive query params | |
| val url = call.request.url | |
| println("[DEBUG_LOG] 401 Unauthorized: ${url.protocol.name}://${url.host}${url.encodedPath}") | |
| if (tokenManager.handleUnauthorized()) { | |
| // Retry idempotent methods with refreshed token | |
| tokenManager.getValidToken()?.takeIf { it.isNotBlank() }?.let { newToken -> | |
| request.headers.remove(HttpHeaders.Authorization) | |
| request.headers.append(HttpHeaders.Authorization, "Bearer $newToken") | |
| } | |
| if (request.method in listOf(HttpMethod.Get, HttpMethod.Head)) { | |
| call = execute(request) | |
| } | |
| } else { | |
| // Refresh failed → force logout + notify UI | |
| runCatching { tokenManager.forceLogout() } | |
| runCatching { | |
| bose.ankush.network.auth.events.AuthEventBus.emit( | |
| bose.ankush.network.auth.events.AuthEvent.Unauthorized( | |
| message = "For security, please log in again to continue using the app." | |
| ) | |
| ) | |
| }.onFailure { e -> | |
| println("[DEBUG_LOG] Failed to emit Unauthorized event: ${e.message}") | |
| } | |
| } | |
| } | |
| call | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt
around lines 21-73, replace the blocking runBlocking-based
defaultRequest/HttpResponseValidator/ResponseObserver logic with a suspendable
HttpSend interceptor: attach the latest token by calling
tokenManager.getValidToken() (suspend) before sending each request, on responses
detect 401, redact query params in logs, call tokenManager.handleUnauthorized()
(suspend) and if it returns true retry idempotent methods (GET/HEAD) with the
refreshed token, otherwise call tokenManager.forceLogout() and emit the
Unauthorized event; ensure TokenManager implements single-flight refresh (e.g.,
Mutex + deferred result) to prevent concurrent refreshes, remove runBlocking
usages and related imports after refactor, and keep all logging and exception
handling non-blocking and concise.
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt
Show resolved
Hide resolved
network/src/commonMain/kotlin/bose/ankush/network/auth/repository/AuthRepositoryImpl.kt
Show resolved
Hide resolved
This commit integrates the `payment` module directly into the `app` module, adds a GitHub Actions CI workflow, and significantly enhances the payment functionality and UI.
- **Payment Module Integration & Removal:**
- The `payment` module has been removed. Its functionality, including Razorpay SDK integration and UI, is now part of the `app` module.
- `PaymentScreen.kt`, `PaymentActivity`, and related resources from the `payment` module have been deleted.
- Razorpay dependency (`com.razorpay:checkout`) is now declared in `app/build.gradle.kts`.
- **Payment Flow Enhancements (App & Network):**
- **New Payment Models:** Added `CreateOrderRequest`, `CreateOrderResponse`, `VerifyPaymentRequest`, `VerifyPaymentResponse`, and related data classes in the `network` module. `CreateOrderResponse` includes a helper `extractData()` to safely parse response.
- **Payment API Service & Repository:**
- Introduced `PaymentApiService` (Ktor implementation: `KtorPaymentApiService`) and `PaymentRepository` (implementation: `PaymentRepositoryImpl`) in the `network` module for creating orders and verifying payments.
- These services are now available via `createPaymentRepository` in `network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt`.
- **ViewModel Logic (MainViewModel):**
- Added `CreateOrder` and `VerifyPayment` use cases.
- New state `PaymentUiState` and `PaymentEvent` (for `LaunchCheckout`) to manage payment flow.
- `startPayment()`: Creates an order via the repository and emits `LaunchCheckout` event.
- `verifyPayment()`: Verifies payment details with the backend.
- `onPaymentFailed()`: Handles payment errors.
- Saves premium status and expiry to `PreferenceManager` upon successful verification.
- **Activity Integration (MainActivity):**
- Implements `PaymentResultWithDataListener` for Razorpay callbacks.
- Launches Razorpay checkout when `PaymentEvent.LaunchCheckout` is received from `MainViewModel`.
- Calls `viewModel.verifyPayment()` on success or `viewModel.onPaymentFailed()` on error.
- Manages `razorpayCheckout` instance lifecycle.
- **UI (SettingsScreen & PremiumBottomSheetContent):**
- New `PremiumBottomSheetContent.kt` in `sunriseui` for the premium subscription UI, now used in `SettingsScreen`.
- `SettingsScreen` now observes `paymentUiState` from `MainViewModel`.
- Displays dynamic content based on payment stage (e.g., "Creating order...", "Verifying payment...", "Premium Activated", error messages).
- Shows an animated "Premium Active" card at the top when subscription is active.
- Premium card in settings shows progress during payment and status (active/failed).
- Subscribe button in bottom sheet now triggers `viewModel.startPayment()`.
- **CI Workflow:**
- Added `.github/workflows/ci.yml` for GitHub Actions.
- Includes steps for checkout, JDK setup, Gradle setup, build, Spotless check (soft), and Detekt (soft).
- **KMM & Dependency Updates:**
- **`KmmDeps.kt`:** Updated versions for `kotlinxSerialization`, `kotlinxCoroutines`, and `kotlinxDateTime`.
- **`Versions.kt`:** Updated versions for Kotlin, Compose BOM, buildGradle, navigation, ktLint, Google Services, various testing libraries (truth, turbine, coroutineTest), core Android libraries, Room, Gson, Firebase BOM, Coroutines, Hilt, Coil, LeakCanary, and Razorpay.
- **`network/build.gradle.kts` & `storage/build.gradle.kts`:** Changed `jvmTarget` to use `JvmTarget.JVM_17`.
- **`app/build.gradle.kts`:**
- Added `kapt { correctErrorTypes = true }`.
- Added Room runtime dependencies (`room-runtime`, `room-ktx`).
- Added `hilt-androidX-compiler`.
- **Refinements & Fixes:**
- **AuthEventBus:** Introduced global `AuthEvent.Unauthorized` in `network` module, observed by `MainActivity` to show a snackbar on forced logout.
- **PreferenceManager:** Added `savePremiumStatus`, `IS_PREMIUM`, and `PREMIUM_EXPIRY` keys.
- **LocaleHelper:** Uses `Locale.forLanguageTag()` for better language code parsing.
- **LoginScreen:** Improved clickable text handling for terms/privacy links using `BasicText` and `pointerInput`.
- **AndroidHttpClient:** Changed Ktor logging to use `android.util.Log.d`.
- **StorageModule:** Hilt DI for storage (`StorageModule.kt`) moved from `storage` to `app` module to decouple `storage` from Hilt. `TokenStorageImpl` and `WeatherStorageImpl` no longer use `@Inject`.
- **Build Script:** Updated `./gradlew deepClean` to use `layout.buildDirectory`.
- **SettingsScreen:** General UI polish, added `EnterAnimated` composable for consistent entry animations.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt (1)
26-33: Ensure secret redaction and consistent log levels across all platformsThe Logging plugin is installed in three places—Android, iOS (default INFO), and commonMain (currently at ALL). We need to prevent sensitive headers (Authorization, Cookie, x-api-key) from being logged and align log priorities, especially since LogLevel.ALL will include headers and bodies.
• AndroidHttpClient.kt (lines 26–33)
- Change
Log.dtoLog.ito matchLogLevel.INFO.- Add
sanitizeHeader { … }to redactHttpHeaders.Authorization,HttpHeaders.Cookie, and"x-api-key".- Import
io.ktor.http.HttpHeaders.Suggested diff:
install(Logging) { logger = object : Logger { override fun log(message: String) { - Log.d("Ktor Android:", message) + Log.i("Ktor Android", message) } } + // Redact sensitive headers if the level changes (e.g., to HEADERS/BODY) + sanitizeHeader { header -> + header == HttpHeaders.Authorization || + header == HttpHeaders.Cookie || + header.equals("x-api-key", ignoreCase = true) + } level = LogLevel.INFO }• NetworkModule.kt (commonMain) at both installations (lines 130–133 and 162–165)
- Currently uses
Logger.SIMPLEwithLogLevel.ALL, which will log headers/bodies in all builds.- Insert the same
sanitizeHeader { … }block beforelevel.- Consider changing to
LogLevel.INFOor gatingLogLevel.ALLto debug builds via a flag.Example:
install(Logging) { logger = Logger.SIMPLE + // Prevent leaking tokens when using ALL + sanitizeHeader { header -> + header == HttpHeaders.Authorization || + header == HttpHeaders.Cookie || + header.equals("x-api-key", ignoreCase = true) + } - level = LogLevel.ALL + level = if (BuildConfig.DEBUG) LogLevel.ALL else LogLevel.INFO }• Optional: Gate all plugin installations behind a
BuildConfig.DEBUGcheck (or DI flag) so that in release buildslevel = LogLevel.NONEor at most INFO.These changes will protect tokens from being logged and maintain consistent log priorities across Android, iOS, and shared code.
buildSrc/src/main/kotlin/KmmDeps.kt (1)
13-18: Removektor-client-serialization(1.x artifact) and rely on ContentNegotiation +ktor-serialization-kotlinx-json.Ktor 2.x replaces
ktor-client-serializationwithktor-client-content-negotiationandktor-serialization-kotlinx-json. Keeping the 1.x artifact can cause resolution errors or duplication. (ktor.io)Proposed changes:
- const val ktorSerialization = "io.ktor:ktor-client-serialization:${KmmVersions.ktor}" const val ktorContentNegotiation = "io.ktor:ktor-client-content-negotiation:${KmmVersions.ktor}" const val ktorJson = "io.ktor:ktor-serialization-kotlinx-json:${KmmVersions.ktor}" ... fun DependencyHandler.addKmmCommonDependencies() { implementation(KmmDeps.ktorCore) - implementation(KmmDeps.ktorSerialization) implementation(KmmDeps.ktorContentNegotiation) implementation(KmmDeps.ktorJson) implementation(KmmDeps.ktorLogging) implementation(KmmDeps.kotlinxSerialization) implementation(KmmDeps.kotlinxCoroutinesCore) implementation(KmmDeps.koinCore) implementation(KmmDeps.kotlinxDateTime) }Also applies to: 36-47
storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (2)
53-74: Preserve coroutine cancellation and add context to errors.Catching Exception will also trap CancellationException. Re-throw cancellations to avoid leaking work and misbehaving structured concurrency.
Apply:
- } catch (e: Exception) { - // If there's an error, throw a more specific IOException with detailed information - throw IOException("Failed to refresh weather data for coordinates (${coordinates.first}, ${coordinates.second}): ${e.message}", e) - } + } catch (e: Throwable) { + if (e is kotlinx.coroutines.CancellationException) throw e + throw IOException( + "Failed to refresh weather data for coordinates (${coordinates.first}, ${coordinates.second}): ${e.message}", + e + ) + }
161-181: Same for hourly: prevent nullable elements with mapNotNull.Align hourly mapping with daily to avoid List<Hourly?>.
- hourly = data?.hourly?.map { hourly -> - hourly?.let { it -> + hourly = data?.hourly?.mapNotNull { hourly -> + hourly?.let { it -> WeatherEntity.Hourly( clouds = it.clouds, dt = it.dt?.toLong(), feels_like = it.feelsLike, humidity = it.humidity, temp = it.temp, weather = it.weather?.mapNotNull { info -> info?.let { Weather( description = it.description, icon = it.icon, id = it.id, main = it.main ) } } ) } - }, + },
♻️ Duplicate comments (7)
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (3)
33-49: Don’t hijack the engine — use the platform client instance directly
HttpClient(createPlatformHttpClient(json).engine)leaks the temporary client and drops engine/plugins installed in platform actuals (timeouts, Logging, ContentNegotiation variants).fun createBasicHttpClient(): HttpClient { val json = Json { ignoreUnknownKeys = true isLenient = true prettyPrint = false encodeDefaults = true } - return HttpClient(createPlatformHttpClient(json).engine) { - install(ContentNegotiation) { - json(json) - } - } + // Preserve platform-installed engine and plugins + return createPlatformHttpClient(json) }
111-135: Same engine-hijack in authenticated client — extend the platform client via config{}Creating a new client from another client’s engine discards platform plugins and risks leaking the original client.
fun createAuthenticatedHttpClient(tokenManager: TokenManager): HttpClient { val json = Json { ignoreUnknownKeys = true; isLenient = true; prettyPrint = false; encodeDefaults = true } - - // Create a platform-specific HttpClient with authentication configuration - return HttpClient(createPlatformHttpClient(json).engine) { - // Install ContentNegotiation plugin - install(ContentNegotiation) { json(json) } - - // Add authentication configuration with token refresh - configureAuth(tokenManager) - - // Install Logging plugin - install(Logging) { - logger = Logger.SIMPLE // Ensures logs are printed to stdout - level = LogLevel.ALL - } - } + val base = createPlatformHttpClient(json) + return base.config { + // Platform client already sets ContentNegotiation/Logging; avoid double-install. + configureAuth(tokenManager) + } }
143-167: Legacy overload repeats the engine-hijack — extend instead of re-createSame rationale as above; extend the platform client directly.
fun createAuthenticatedHttpClient(tokenStorage: TokenStorage): HttpClient { val json = Json { ignoreUnknownKeys = true; isLenient = true; prettyPrint = false; encodeDefaults = true } - - // Create a platform-specific HttpClient with authentication configuration - return HttpClient(createPlatformHttpClient(json).engine) { - // Install ContentNegotiation plugin - install(ContentNegotiation) { json(json) } - - // Add authentication configuration - configureAuth(tokenStorage) - - // Install Logging plugin - install(Logging) { - logger = Logger.SIMPLE // Ensures logs are printed to stdout - level = LogLevel.ALL - } - } + val base = createPlatformHttpClient(json) + return base.config { + configureAuth(tokenStorage) + } }app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (2)
176-184: Good fix: side-effects moved out of composition and keyed correctlyFetching location is now wrapped in LaunchedEffect and notification banner state is keyed on
.value. This addresses previous re-composition pitfalls.Also applies to: 185-193
247-254: Good catch: permanently-declined logic fixed
isPermanentlyDeclined = !shouldShowRequestPermissionRationale(permission)is correct; previously this was inverted.buildSrc/src/main/java/Versions.kt (2)
56-56: Coroutines version still needs upgrade for Kotlin 2.2.0 compatibility.The coroutines version has been updated to 1.9.0, but this is still insufficient for Kotlin 2.2.0. Based on previous analysis, Kotlin 2.x requires kotlinx-coroutines 1.8.0+ minimum, and the latest stable version is 1.10.2.
Apply this diff to upgrade to the latest compatible version:
- const val coroutines = "1.9.0" + const val coroutines = "1.10.2"
7-7: Compose BOM version mismatch - ecosystem libraries need alignment.The Compose BOM has been updated to "2025.06.01", but ecosystem libraries like
accompanist,coilCompose, andlifecyclemay need version alignment to avoid compatibility issues.Based on the current versions in this file:
accompanist = "0.36.0"(Line 42) - appears updated appropriatelycoilCompose = "2.6.0"(Line 64) - appears updated appropriatelylifecycle = "2.7.0"(Line 39) - appears updated appropriatelyThe versions look better aligned now compared to the previous review, but verify compatibility with the 2025.06.01 BOM.
🧹 Nitpick comments (39)
language/src/main/java/bose/ankush/language/util/LocaleHelper.kt (1)
9-19: Uppercasing with Locale.getDefault() can mis-case for some locales (e.g., Turkish).For stable country-code uppercasing, prefer Locale.ROOT to avoid locale-specific rules:
- val countryCode = this.split("-").lastOrNull()?.uppercase(Locale.getDefault()) ?: return "" + val countryCode = this.split("-").lastOrNull()?.uppercase(Locale.ROOT) ?: return ""sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt (2)
108-111: Prefer theme colors over hard-coded hex to support dark mode and branding.Replace
Color(0xFFFFB74D)withMaterialTheme.colorScheme.primary(or a dedicatedpremiumColorin your theme):- colors = ButtonDefaults.buttonColors(containerColor = Color(0xFFFFB74D)) + colors = ButtonDefaults.buttonColors(containerColor = MaterialTheme.colorScheme.primary) ... - .background(Color(0xFFFFB74D)) + .background(MaterialTheme.colorScheme.primary)Also applies to: 166-167
141-148: Use TextButton for “No Thanks” for better semantics and touch feedback.Clickable Text lacks built-in button semantics and ripple. Replace with
TextButton:- Text( - text = "No Thanks", - ..., - modifier = Modifier - .clickable { onDismiss() } - .padding(vertical = 8.dp) - ) + TextButton(onClick = onDismiss) { + Text( + text = "No Thanks", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.primary + ) + }app/src/main/java/bose/ankush/weatherify/presentation/payment/PaymentViewModel.kt (2)
3-9: File name is misleading; consider renaming to reflect contents (no ViewModel inside).The file only declares UI models and events. Rename to PaymentUiModels.kt (or similar) to avoid confusion and improve discoverability.
13-19: Avoid duplicated state sources: derive loading from stage.Keeping both loading and stage invites drift (e.g., stage=Verifying but loading=false). Prefer deriving loading in UI from stage to keep a single source of truth.
Apply this minimal change and derive loading where consumed:
data class PaymentUiState( - val loading: Boolean = false, val message: String? = null, val stage: PaymentStage = PaymentStage.Idle, val isPremiumActivated: Boolean = false, val expiryMillis: Long? = null )Optionally add an extension for convenience (usage-site only):
val PaymentUiState.isLoading get() = stage == PaymentStage.CreatingOrder || stage == PaymentStage.Verifyingstorage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (2)
35-46: Coordinates are unused; either suppress or model multi-location in storage.Both getWeatherReport and getAirQualityReport ignore coordinates. If storage is intentionally single-location, add @Suppress("UNUSED_PARAMETER") and document it. If multi-location is desired, include lat/lon in DAO lookups.
Example:
-override fun getWeatherReport(coordinates: Pair<Double, Double>): Flow<Any?> { +@Suppress("UNUSED_PARAMETER") +override fun getWeatherReport(coordinates: Pair<Double, Double>): Flow<Any?> { return weatherDatabase.weatherDao().getWeather() }Apply similarly to getAirQualityReport.
91-121: Timestamp normalization consistency.dt fields are normalized to Long; sunrise/sunset remain as-is. Double-check Room column types; if columns are Long, convert sunrise/sunset to Long as well for current and daily.
If needed:
- sunrise = current.sunrise, - sunset = current.sunset, + sunrise = current.sunrise?.toLong(), + sunset = current.sunset?.toLong(), ... - sunrise = it.sunrise, - sunset = it.sunset, + sunrise = it.sunrise?.toLong(), + sunset = it.sunset?.toLong(),Also applies to: 126-158, 164-179
network/src/commonMain/kotlin/bose/ankush/network/model/PaymentModels.kt (1)
11-18: Nit: annotate optional request fields with documentation of backend expectations.partial_payment and first_payment_min_amount behavior varies by provider. Add KDoc noting units and constraints to guide API usage.
.github/workflows/ci.yml (3)
31-38: Soft-fail is fine for adoption; plan to enforce and surface reports.Consider uploading Spotless/Detekt reports as artifacts and flip to hard-fail after the first cleanup commit.
Example:
- name: Spotless check (soft enforcement for initial adoption) run: ./gradlew --no-daemon spotlessCheckAll continue-on-error: true + - name: Upload Spotless reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: spotless-reports + path: '**/build/reports/spotless/*.*/**/*' - name: Detekt (soft enforcement for initial adoption) run: ./gradlew --no-daemon detektAll continue-on-error: true + - name: Upload Detekt reports + if: always() + uses: actions/upload-artifact@v4 + with: + name: detekt-reports + path: '**/build/reports/detekt/**'Also applies to: 39-42
3-8: YAML trigger nit: pull_request branches filter is redundant.branches: ["**"] for pull_request is unnecessary. Omitting defaults to all branches and is cleaner.
on: pull_request: - branches: [ "**" ] push: branches: [ "main" ]
9-12: Add concurrency group to auto-cancel superseded runs.Avoid wasting runners on stale commits.
jobs: build: runs-on: ubuntu-latest + concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: trueapp/src/main/java/bose/ankush/weatherify/data/preference/PreferenceManager.kt (1)
33-38: savePremiumStatus: defensive defaulting on expiry.If expiryMillis can be 0L or negative from callers, consider normalizing to null or clamping to >= currentTimeMillis to avoid treating expired subscriptions as valid.
Example:
- override suspend fun savePremiumStatus(isPremium: Boolean, expiryMillis: Long) { + override suspend fun savePremiumStatus(isPremium: Boolean, expiryMillis: Long) { context.dataStore.edit { preferences -> preferences[PreferenceManager.IS_PREMIUM] = isPremium - preferences[PreferenceManager.PREMIUM_EXPIRY] = expiryMillis + preferences[PreferenceManager.PREMIUM_EXPIRY] = expiryMillis.coerceAtLeast(0L) } }If 0 means “no expiry,” align with the domain contract and store null instead using nullable key semantics.
network/src/commonMain/kotlin/bose/ankush/network/api/KtorPaymentApiService.kt (2)
25-28: Harden URL construction to avoid double slashesIf
baseUrlis configured with a trailing slash at runtime, interpolations like"$baseUrl/create-order"and"$baseUrl/store-payment"can produce//and lead to subtle cache/proxy issues. Trim trailing slashes before appending the path.Apply this diff:
- httpClient.post("$baseUrl/create-order") { + httpClient.post("${baseUrl.trimEnd('/')}/create-order") { contentType(ContentType.Application.Json) setBody(request) }.body()- httpClient.post("$baseUrl/store-payment") { + httpClient.post("${baseUrl.trimEnd('/')}/store-payment") { contentType(ContentType.Application.Json) setBody(request) }.body()Also applies to: 34-37
23-31: Map HTTP failures to a domain-specific error (optional)Consider normalizing non-2xx responses and deserialization failures into a project-wide
NetworkException(or similar) so callers aren’t coupled to Ktor exceptions. If this is already handled by an HttpResponseValidator or a global plugin, ignore.Also applies to: 32-39
app/src/main/java/bose/ankush/weatherify/domain/preference/PreferenceManager.kt (3)
11-14: Method name no longer matches intent; consider deprecating or renamingDocs say “preferences (location, premium, etc.)” but the method is still
getLocationPreferenceFlow(). Consider addinggetPreferenceFlow()and deprecating the old name to avoid confusion across the codebase.If you want to keep source compatibility, you can do this in the interface:
- fun getLocationPreferenceFlow(): Flow<Preferences> + fun getPreferenceFlow(): Flow<Preferences> + + @Deprecated("Use getPreferenceFlow()") + fun getLocationPreferenceFlow(): Flow<Preferences> = getPreferenceFlow()
21-25: Clarify semantics for premium state persistenceAdd KDoc for time base and expected units to avoid ambiguity across platforms (epoch millis in UTC), and note whether clearing premium requires passing
isPremium=falseandexpiryMillis=0L.- /** - * Save premium subscription status and expiry - */ - suspend fun savePremiumStatus(isPremium: Boolean, expiryMillis: Long) + /** + * Save premium subscription status and expiry. + * + * @param isPremium true if user currently has an active premium entitlement. + * @param expiryMillis absolute expiry timestamp in epoch milliseconds (UTC). Use 0L to clear. + */ + suspend fun savePremiumStatus(isPremium: Boolean, expiryMillis: Long)
32-35: Keys look good; add brief KDoc for cross-module discoverabilityA short comment helps future contributors understand expected value semantics and units (boolean flag; epoch millis).
- val IS_PREMIUM = androidx.datastore.preferences.core.booleanPreferencesKey("is_premium") - val PREMIUM_EXPIRY = - androidx.datastore.preferences.core.longPreferencesKey("premium_expiry") + /** Whether the user currently has a premium entitlement */ + val IS_PREMIUM = androidx.datastore.preferences.core.booleanPreferencesKey("is_premium") + /** Absolute expiry timestamp in epoch millis (UTC) */ + val PREMIUM_EXPIRY = + androidx.datastore.preferences.core.longPreferencesKey("premium_expiry")storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt (2)
1-3: Entire file is wrapped in a block comment — consider deleting or renaming to a clear placeholderAs-is, nothing in this file is compiled (which may be intentional). To reduce confusion, either delete the file or rename it to
StorageModulePlaceholder.ktwith a short top-level KDoc explaining that DI moved to the app module.Also applies to: 76-78
7-7: Dead imports inside a commented fileSince the whole file is commented, these imports are inert. If you keep the placeholder, consider removing the embedded code altogether to avoid drift.
Also applies to: 10-10, 14-14, 22-22
network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepository.kt (1)
8-21: Interface looks clean; verify domain exposureThe API mirrors network models directly. If your architecture prefers domain types or a Result wrapper for failures, consider adapting here or in the impl. Otherwise, this is fine for a thin network repository.
network/src/commonMain/kotlin/bose/ankush/network/api/PaymentApiService.kt (1)
8-21: API surface is straightforward and cohesiveThe service cleanly models the two payment operations. Consider documenting thrown exceptions (e.g.,
NetworkException) if you standardize error mapping, so callers know what to catch.network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt (3)
18-31: DRY the connectivity guard with a small helper and centralize the messageBoth methods duplicate the connectivity check and the error message. A tiny helper trims repetition and makes changes (e.g., localization) one-spot.
Apply this diff within this file:
class PaymentRepositoryImpl( private val apiService: PaymentApiService, private val networkConnectivity: NetworkConnectivity ) : PaymentRepository { + private companion object { + private const val NO_NETWORK_MESSAGE = "No internet connection" + } + + private suspend inline fun <T> withNetwork(crossinline block: suspend () -> T): T { + if (!networkConnectivity.isNetworkAvailable()) { + throw IllegalStateException(NO_NETWORK_MESSAGE) + } + return block() + } + override suspend fun createOrder(request: CreateOrderRequest): CreateOrderResponse { - if (!networkConnectivity.isNetworkAvailable()) { - throw IllegalStateException("No internet connection") - } - return apiService.createOrder(request) + return withNetwork { apiService.createOrder(request) } } override suspend fun verifyPayment(request: VerifyPaymentRequest): VerifyPaymentResponse { - if (!networkConnectivity.isNetworkAvailable()) { - throw IllegalStateException("No internet connection") - } - return apiService.verifyPayment(request) + return withNetwork { apiService.verifyPayment(request) } } }
18-23: Use a domain-specific exception (or Result) instead of IllegalStateException for offline casesIllegalStateException implies a programmer error; offline is an expected runtime condition. Consider a small domain exception (e.g., NoNetworkException) in commonMain or return a Result to avoid exception control flow across KMP boundaries.
I can sketch a minimal NoNetworkException in commonMain and a Result-based facade if you’d like.
18-31: Wrap/translate API exceptions at repository boundaryRight now, any Ktor/engine exceptions bubble up. Consider mapping to a sealed PaymentError (e.g., Network, Unauthorized, Server, Unknown) so callers don’t depend on transport details.
If desired, I can add a thin mapper here and generate unit tests to verify the mapping behavior.
storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt (1)
24-28: Consider using DAO Flow for reads to avoid a separate “hasToken” pathIf DAO can expose a Flow<AuthToken?>, you can derive both “value” and “presence” consistently from one stream (e.g., map { it?.token } for value, map { it != null } for presence). Keeps sources of truth singular.
Happy to propose DAO/impl changes if you want to consolidate.
storage/build.gradle.kts (1)
105-109: kapt scoped correctly to Room; keep it Android-onlyGood separation. Consider adding “kaptTest(RoomCompiler)” only if you generate Room entities in tests (likely unnecessary).
build.gradle.kts (2)
35-52: Prefer registering deepClean as a Delete taskUsing Gradle’s Delete task gives incremental logging and up-to-date checks. Also consider including buildSrc/build if present.
-tasks.register("deepClean") { +tasks.register<Delete>("deepClean") { description = "Cleans every module and removes all build artefacts in this repo." group = "build setup" - - // Run each subproject's own clean task first (honors plugin-specific clean hooks) - dependsOn(subprojects.map { "${it.path}:clean" }) - - doLast { - val dirsToDelete = mutableSetOf<File>().apply { - allprojects.forEach { add(it.layout.buildDirectory.get().asFile) } - add(rootProject.layout.projectDirectory.dir(".gradle").asFile) - } - delete(dirsToDelete) - } + // Run each subproject's clean first (honors plugin-specific hooks) + dependsOn(subprojects.map { "${it.path}:clean" }) + val dirsToDelete = mutableSetOf<File>().apply { + allprojects.forEach { add(it.layout.buildDirectory.get().asFile) } + add(rootProject.layout.projectDirectory.dir(".gradle").asFile) + // Also clean buildSrc if present + add(rootProject.layout.projectDirectory.dir("buildSrc/build").asFile) + } + delete(dirsToDelete) }
19-24: Plugin set: ktlint Gradle plugin declared but not appliedFine to keep for optional use, but consider removing if Spotless fully replaces it to reduce plugin resolution time.
app/build.gradle.kts (2)
3-13: Modernize Kotlin plugin IDsUse the canonical plugin IDs to align with Kotlin 1.9+/2.x conventions.
plugins { - id("com.android.application") - id("kotlin-android") - id("kotlin-kapt") + id("com.android.application") + id("org.jetbrains.kotlin.android") + id("org.jetbrains.kotlin.kapt") id("com.google.gms.google-services") id("dagger.hilt.android.plugin") id("kotlin-parcelize") id("com.google.android.libraries.mapsplatform.secrets-gradle-plugin") id("com.github.ben-manes.versions") id("org.jetbrains.kotlin.plugin.compose") }
165-167: Nit: prefer opt-in DSL over raw compiler flags when possibleWhere supported, prefer
optIn.addAll(...)over manual-Xopt-inflags to keep the build script declarative.kotlin { compilerOptions { jvmTarget.set(JvmTarget.JVM_17) - freeCompilerArgs.addAll( - "-Xopt-in=kotlin.RequiresOptIn", - "-Xopt-in=androidx.compose.animation.ExperimentalAnimationApi" - ) + optIn.add("kotlin.RequiresOptIn") + optIn.add("androidx.compose.animation.ExperimentalAnimationApi") } }network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (1)
119-134: Avoid double-installing Logging/ContentNegotiationPlatform clients already install these plugins (see Android/iOS actuals). Re-installing can cause plugin reinstallation issues or diverging config.
- Rely on platform installs; only add auth here.
- If you need to override logging level, use
plugin(Logging).logger = ...inconfig {}rather than re-installing.Also applies to: 151-166
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (2)
295-297: Gate POST_NOTIFICATIONS request by API 33+Requesting notification permission on pre-33 devices is unnecessary. Guard by SDK level.
- LaunchedEffect(Unit) { - notificationPermissionResultLauncher.launch(ACCESS_NOTIFICATION) - } + LaunchedEffect(Unit) { + if (android.os.Build.VERSION.SDK_INT >= 33) { + notificationPermissionResultLauncher.launch(ACCESS_NOTIFICATION) + } else { + // Permission not required; ensure UI reflects that + viewModel.updateShowNotificationBannerState(false) + } + }
133-149: Minor: preload Razorpay once in Application.onCreate()Preloading each time before checkout is harmless but redundant. Consider moving
Checkout.preload(appContext)to your Application class.Additionally, consider calling
Checkout.clearUserData(context)after success/failure to clear cached user data.override fun onPaymentSuccess(razorpayPaymentID: String?, paymentData: PaymentData?) { @@ - razorpayCheckout = null + razorpayCheckout = null + Checkout.clearUserData(this) } override fun onPaymentError(code: Int, response: String?, paymentData: PaymentData?) { @@ - razorpayCheckout = null + razorpayCheckout = null + Checkout.clearUserData(this) }app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (4)
88-92: Avoid IO/JSON work on every recomposition — remember the language listLoading from JSON should be wrapped in remember to prevent repeated work during recompositions.
- val languageList = LocaleConfigMapper.getAvailableLanguagesFromJson( - jsonFile = "countryConfig.json", - context = LocalContext.current - ) + val context = LocalContext.current + val languageList = remember(context) { + LocaleConfigMapper.getAvailableLanguagesFromJson( + jsonFile = "countryConfig.json", + context = context + ) + }
221-226: Reuse date formatter in composition
SimpleDateFormatis relatively heavy. Wrap it inrememberto avoid recreating.- val df = SimpleDateFormat( - "MMM d, yyyy", - Locale.getDefault() - ) + val df = remember { SimpleDateFormat("MMM d, yyyy", Locale.getDefault()) }Apply at both expiry text usages.
Also applies to: 535-537
79-79: Remove unnecessary suppression
@Suppress("KotlinConstantConditions")appears unnecessary here; avoid broad suppressions to keep warnings meaningful.
631-639: Disable Logout card while logging outPrevent repeated taps during logout progress.
- .clickable { showLogoutDialog.value = true }, + .clickable(enabled = !isLoggingOut) { showLogoutDialog.value = true },app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (2)
283-289: Unify error messaging for auth flowsLogin/Register/Logout use raw
e.message, which can be empty or unfriendly. You already haveerrorResponseFromException(...). Use it for consistent UX.- } catch (e: Exception) { - _authState.value = AuthState.Error(UiText.DynamicText(e.message ?: "Login failed")) - } + } catch (e: Exception) { + _authState.value = AuthState.Error(errorResponseFromException(e)) + }Apply similarly to register/logout catch blocks.
Also applies to: 292-312, 325-327
137-147: Minor: avoid redundant updates with distinctUntilChangedIf
isLoggedIn()emits the same value repeatedly, you’ll re-triggerisAuthInitializedlogic and downstream UI updates.- authRepository.isLoggedIn().collectLatest { loggedIn -> + authRepository.isLoggedIn().distinctUntilChanged().collectLatest { loggedIn -> _isLoggedIn.value = loggedIn if (!initialized) { _isAuthInitialized.value = true initialized = true } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (38)
.github/workflows/ci.yml(1 hunks)app/build.gradle.kts(5 hunks)app/src/main/java/bose/ankush/weatherify/data/preference/PreferenceManager.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/domain/preference/PreferenceManager.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt(6 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/presentation/payment/PaymentViewModel.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt(12 hunks)build.gradle.kts(2 hunks)buildSrc/src/main/java/Dependencies.kt(2 hunks)buildSrc/src/main/java/Versions.kt(1 hunks)buildSrc/src/main/kotlin/KmmDeps.kt(3 hunks)language/src/main/java/bose/ankush/language/util/LocaleHelper.kt(1 hunks)local.properties(1 hunks)network/build.gradle.kts(5 hunks)network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/api/KtorPaymentApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/api/PaymentApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/auth/events/AuthEvents.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/PaymentModels.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepository.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt(1 hunks)payment/build.gradle.kts(0 hunks)payment/proguard-rules.pro(0 hunks)payment/src/androidTest/java/bose/ankush/payment/ExampleInstrumentedTest.kt(0 hunks)payment/src/main/AndroidManifest.xml(0 hunks)payment/src/main/java/bose/ankush/payment/PaymentScreen.kt(0 hunks)payment/src/main/res/values/strings.xml(0 hunks)payment/src/test/java/bose/ankush/payment/ExampleUnitTest.kt(0 hunks)settings.gradle.kts(0 hunks)storage/build.gradle.kts(6 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/di/StorageModule.kt(2 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt(1 hunks)storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt(7 hunks)sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt(1 hunks)sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt(1 hunks)
💤 Files with no reviewable changes (8)
- payment/proguard-rules.pro
- payment/src/androidTest/java/bose/ankush/payment/ExampleInstrumentedTest.kt
- payment/src/test/java/bose/ankush/payment/ExampleUnitTest.kt
- payment/src/main/res/values/strings.xml
- settings.gradle.kts
- payment/build.gradle.kts
- payment/src/main/java/bose/ankush/payment/PaymentScreen.kt
- payment/src/main/AndroidManifest.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt
- buildSrc/src/main/java/Dependencies.kt
- sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt
🧰 Additional context used
🧬 Code graph analysis (6)
app/build.gradle.kts (1)
buildSrc/src/main/kotlin/KmmDeps.kt (1)
implementation(59-61)
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (3)
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt (1)
createPlatformHttpClient(16-35)network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt (1)
createPlatformHttpClient(15-36)network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt (2)
configureAuth(20-73)configureAuth(79-88)
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (1)
language/src/main/java/bose/ankush/language/presentation/LanguageScreen.kt (1)
ScreenHeader(140-186)
network/build.gradle.kts (1)
buildSrc/src/main/kotlin/KmmDeps.kt (1)
implementation(59-61)
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (9)
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (1)
errorResponseFromException(50-55)storage/src/androidMain/kotlin/bose/ankush/storage/room/WeatherDao.kt (1)
getAirQuality(32-33)storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (1)
getWeatherReport(35-37)app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.kt (1)
getWeatherReport(31-34)app/src/main/java/bose/ankush/weatherify/domain/repository/WeatherRepository.kt (1)
getWeatherReport(16-16)network/src/commonMain/kotlin/bose/ankush/network/api/KtorPaymentApiService.kt (2)
createOrder(23-30)verifyPayment(32-39)network/src/commonMain/kotlin/bose/ankush/network/api/PaymentApiService.kt (3)
createOrder(11-21)createOrder(15-15)verifyPayment(20-20)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepository.kt (3)
createOrder(11-21)createOrder(15-15)verifyPayment(20-20)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt (2)
createOrder(18-23)verifyPayment(25-30)
app/src/main/java/bose/ankush/weatherify/presentation/MainActivity.kt (3)
sunriseui/src/main/java/bose/ankush/sunriseui/components/GlassmorphicSnackbar.kt (1)
rememberGlassmorphicSnackbarState(109-131)sunriseui/src/main/java/bose/ankush/sunriseui/auth/LoginScreen.kt (1)
LoginScreen(65-412)app/src/main/java/bose/ankush/weatherify/base/permissions/PermissionAlertDialog.kt (1)
PermissionAlertDialog(8-27)
🪛 Gitleaks (8.27.2)
local.properties
9-9: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qodana
- GitHub Check: build
🔇 Additional comments (28)
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt (2)
23-25: Good fix: register standard JSON once (no mixed MIME parsing).Removing the mixed
ContentType.parse("application/json, text/html; charset=UTF-8")path and keeping a singlejson(json)is correct and avoids construction-time errors while matching Ktor defaults. This also keeps behavior consistent with iOS.
3-3: Switch to android.util.Log is appropriate.Using Logcat instead of println improves visibility and tagging on Android. No issues.
local.properties (1)
9-10: Secrets removed from VCS – please finalize rotation and history purgeThe sensitive tokens have been stripped from version control and replaced with placeholders, and local.properties is now properly ignored. Please complete the remaining operational steps to fully secure your credentials.
• .localignore check
– .gitignore includeslocal.properties(line 24), so the file won’t be committed again.• Placeholder values only
– local.properties now containsOPEN_WEATHER_API_KEY=your_api_key_here(line 180) instead of a real key.• Runtime injection in place
– Code reads the Razorpay key viaBuildConfig.RAZORPAY_KEY(Kotlin source, around line 366), so no hard-coded secrets remain.Action items:
- Rotate/revoke the exposed API keys in your OpenWeather and Razorpay dashboards immediately.
- Purge all past commits containing the old secrets (e.g. coordinate a
git filter-repo --path local.properties --invert-paths).- Add a pre-commit or CI secret-scanning step to prevent future leaks.
- Continue using environment/CI variables and BuildConfig (or BuildKonfig) for all runtime secrets.
language/src/main/java/bose/ankush/language/util/LocaleHelper.kt (1)
21-25: Double-check intended display language.
locale.getDisplayName(locale)returns the name in its own language (e.g., “Deutsch”). If you want it localized to the app/device language, uselocale.getDisplayName(Locale.getDefault())instead. If self-language is intentional, ignore.buildSrc/src/main/kotlin/KmmDeps.kt (2)
36-47: These helpers target the plain ‘implementation’ configuration; verify usage with Kotlin Multiplatform source sets.In KMP modules, dependencies usually go under
sourceSets { commonMain.dependencies { implementation(...) } }, not the top-levelimplementationbucket. Consider providing overloads forKotlinDependencyHandleror per-source-set functions:// Alternative helper for commonMain: import org.jetbrains.kotlin.gradle.plugin.KotlinDependencyHandler @Suppress("unused") fun KotlinDependencyHandler.addKmmCommonDependencies() { implementation(KmmDeps.ktorCore) implementation(KmmDeps.ktorContentNegotiation) implementation(KmmDeps.ktorJson) implementation(KmmDeps.ktorLogging) implementation(KmmDeps.kotlinxSerialization) implementation(KmmDeps.kotlinxCoroutinesCore) implementation(KmmDeps.koinCore) implementation(KmmDeps.kotlinxDateTime) }If you keep the current
DependencyHandlerhelpers, ensure they’re only used in Android/JVM modules (or aliased to the right configurations likecommonMainImplementationwhen called from multiplatform modules).
5-8: Kotlin plugin version alignment confirmedThe Kotlin Gradle plugin is consistently set to 2.2.0 (see buildSrc/src/main/java/Versions.kt:
const val kotlin = "2.2.0") and is applied in the root build.gradle.kts for bothorg.jetbrains.kotlin.androidandorg.jetbrains.kotlin.multiplatform(lines 14–15). No version catalog is in use.The bumped libraries—kotlinx-serialization 1.7.3, kotlinx-coroutines 1.9.0, Koin 3.5.6, and kotlinx-datetime 0.6.1—are compatible with Kotlin 2.2.0, so no further action is required.
storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (2)
123-160: UsemapNotNullto Prevent Nullable Elements indailyListThe current code
daily = data?.daily?.map { daily -> daily?.let { … } },produces a
List<WeatherEntity.Daily?>?. If yourWeatherEntitydefinesval daily: List<Daily>?(i.e. the list itself may be null, but its elements must be non-null), you’ll end up with a mismatched type (nullable elements) that Room’s TypeConverter may reject or that could introduce unexpected nulls at runtime.
Please update to filter out null items and emit a
List<Daily>?by switching tomapNotNull:- daily = data?.daily?.map { daily -> - daily?.let { + daily = data?.daily?.mapNotNull { daily -> + daily?.let { WeatherEntity.Daily( clouds = it.clouds, dew_point = it.dewPoint, dt = it.dt?.toLong(), humidity = it.humidity, pressure = it.pressure, rain = it.rain, summary = it.summary, sunrise = it.sunrise, sunset = it.sunset, temp = it.temp?.let { temp -> WeatherEntity.Daily.Temp( day = temp.day, eve = temp.eve, max = temp.max, min = temp.min, morn = temp.morn, night = temp.night ) }, uvi = it.uvi, weather = it.weather?.mapNotNull { info -> info?.let { Weather( description = it.description, icon = it.icon, id = it.id, main = it.main ) } }, wind_gust = it.windGust, wind_speed = it.windSpeed ) } - }, + },
- File:
storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt- Lines: 123–160
Action required:
Verify thatWeatherEntitydeclares thedailyproperty asList<Daily>?(elements non-nullable). If so, apply this change to align the mapper with your Room schema and avoid nullable list elements.
59-61: No action required — flow semantics are correctThe network repository’s
getWeatherReportandgetAirQualityReportmethods are backed byMutableStateFlowinstances that are synchronously updated inrefreshWeatherData. BecauserefreshWeatherDatafully completes before the subsequentfirstOrNull()calls, those calls will always observe the freshly written non-null values:
networkRepository.refreshWeatherData(...)(suspend) fetches and updates_weatherDataand_airQualityDataStateFlows before returning.- Calling
getWeatherReport(...).firstOrNull()immediately emits the current_weatherData.value(now non-null), and similarly for air quality.As a result, there is no risk of reading initial nulls, and no changes are needed here.
network/src/commonMain/kotlin/bose/ankush/network/model/PaymentModels.kt (2)
31-62: Enhance CreateOrderResponse.extractData for robust numeric parsing and currency normalizationTo handle cases where backend returns numeric fields as JSON numbers or strings, update the parsing logic in
CreateOrderResponse.extractData:
- Import the necessary extensions:
import kotlinx.serialization.json.contentOrNull + import kotlinx.serialization.json.longOrNull + import kotlinx.serialization.json.doubleOrNull + import kotlin.math.roundToLong- Replace the existing
amountparsing:- val amountStr = obj["amount"]?.jsonPrimitive?.content
- val amount = amountStr?.toLongOrNull() ?: 0L
- val amountPrim = obj["amount"]?.jsonPrimitive
- val amount = amountPrim?.longOrNull
?: amountPrim?.doubleOrNull?.roundToLong()?: 0L
- Normalize the
currencyfield by trimming whitespace and uppercasing:- val currency = obj["currency"]?.jsonPrimitive?.contentOrNull ?: ""
- val currency = obj["currency"]?.jsonPrimitive
?.contentOrNull?.trim()?.uppercase()?: ""• File:
network/src/commonMain/kotlin/bose/ankush/network/model/PaymentModels.kt
• Lines: 31–62Please verify that the backend indeed returns monetary amounts in minor units (e.g., paise or cents) so the UI can correctly format and display subunit values.
76-81: VerifyVerifyPaymentResponse.statusmodeling or confirm boolean contractThe
VerifyPaymentResponsecurrently deserializes"status"directly into a Boolean (success), whereasCreateOrderResponsetreats itsstatusas aJsonElement?to safely handle string, nested, or missing values. Without a strict guarantee from the backend that"status"will always be a JSON boolean, decoding intoBooleanrisks runtime failures.• Location:
– File:network/src/commonMain/kotlin/bose/ankush/network/model/PaymentModels.kt
– Lines: 76–81 (theVerifyPaymentResponsedeclaration)• Recommended change (if
"status"can vary in type):@Serializable -data class VerifyPaymentResponse( - @SerialName("status") val success: Boolean = false, +data class VerifyPaymentResponse( + @SerialName("status") val status: JsonElement? = null, val message: String? = null, val data: VerifyPaymentData? = null ) { + /** Interpret various backend representations of `status`. */ + fun isSuccess(): Boolean { + // Try nested object: { "success": true/false } + val prim = (status as? JsonObject)?.get("success")?.jsonPrimitive + // Or primitive boolean/string + ?: (status as? JsonPrimitive) + return prim?.booleanOrNull + ?: prim?.contentOrNull?.equals("success", ignoreCase = true) == true + // Fallback to data.verified + ?: (data?.verified == true) + } }• If the backend does guarantee a strict JSON boolean for
"status", the existing Boolean property can remain—please verify the API contract..github/workflows/ci.yml (1)
25-27: deepClean task verified—no changes neededThe
deepCleantask is registered inbuild.gradle.kts(lines 37–40), so the CI step will succeed as written:
- File: build.gradle.kts
· tasks.register("deepClean") { … } (lines 37–40)network/src/commonMain/kotlin/bose/ankush/network/api/KtorPaymentApiService.kt (1)
23-31: Good Ktor wiring and JSON body usageThe service correctly sets JSON content type and leverages Ktor’s serialization via
.body(). Wrapping calls with a retry utility is consistent with the rest of the network layer.storage/build.gradle.kts (2)
12-14: Modern JVM target config looks goodSwitch to compilerOptions + JvmTarget.JVM_17 is correct for AGP 8+/Kotlin 2.x.
42-53: Gson usage confirmed – original removal suggestion isn’t applicableA search through the
storagemodule shows that Gson is actively used for JSON parsing and Room type converters, so it cannot be removed without migration work:
- JsonParser.kt defines a
JsonParserclass wrapping aGsoninstance and callsgson.fromJson(...)/gson.toJson(...).- StorageModule.kt provides a
Gsonsingleton and aJsonParservia Dagger.- WeatherDataModelConverters.kt uses
com.google.gson.reflect.TypeTokenfor Room type conversion.Since these files depend on Gson, keeping the current dual-stack setup is necessary unless you choose to migrate all JSON handling in this module to kotlinx-serialization. There are no unused Gson references to remove.
Likely an incorrect or invalid review comment.
build.gradle.kts (2)
53-79: Spotless configuration: solid baselineKtlint CLI + editorConfigOverride and Gradle Kotlin targets look good. This should reduce churn and false positives for @composable names.
80-96: The script will check how the project’s compile and toolchain settings align with the Detekt jvmTarget. Once you have the output, we can confirm whether 21 is appropriate or if we should align everything to 17. Please share the results so we can finalize.network/build.gradle.kts (3)
11-13: Modern jvmTarget configuration is correct for KMP Android targetGood move to compilerOptions + JvmTarget.JVM_17.
46-51: Android Ktor dependency scoped correctly to androidMainThis aligns with KMP best practices. No issues.
59-68: Shared iosMain with Ktor iOS is a clean consolidationThe iosMain hub + per-arch dependsOn is the right pattern.
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (2)
17-17: Verify NetworkConstants package pathImport points to
utils.NetworkConstants, while other modules refer tocommon.NetworkConstants. Ensure the package path is consistent to avoid compile errors.
31-31: No typo in iOS actual –ContentNegotiationis already correctThe iOS‐specific implementation in
network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.ktimports and uses the correctly spelledContentNegotiationplugin:• Import at line 5:
import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
• Usage at line 23:
install(ContentNegotiation) { … }There is no instance of the misspelled
ContentNegotiitionin this file, so no change is required.Likely an incorrect or invalid review comment.
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (1)
414-424: Premium card UI logic reads wellGood state gating (disabled during CreatingOrder/AwaitingPayment/Verifying) and clear progress indication with Animated progress.
Also applies to: 512-546
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (1)
233-258: Data loading pipeline looks solidNice use of combine, flowOn, and structured error handling. The cancellation handling is correct.
buildSrc/src/main/java/Versions.kt (5)
16-20: Well-structured ktLint and detekt version management.The separation of ktLint plugin and CLI versions is a good practice to avoid resolution conflicts. The addition of detekt version constant also improves build configuration consistency.
25-33: Good testing library version updates.The testing dependencies have been appropriately updated:
truthbumped to 1.1.5turbinemajor version bump to 1.0.0coroutineTestaligned with coroutines version at 1.9.0mockWebServerandmockkupdated to recent stable versions
36-44: Comprehensive AndroidX and UI library updates.The core Android libraries have been systematically updated to recent stable versions:
androidCoreto 1.13.1appCompatto 1.7.0androidMaterialto 1.11.0lifecycleto 2.7.0accompanistto 0.36.0dataStoreto 1.1.1These updates should provide better stability and new features.
47-71: Database, networking, and miscellaneous library updates look good.The remaining library updates are well-structured:
- Room bumped to 2.7.0 for better database performance
- Firebase BOM updated to 33.0.0 for latest Firebase features
- Hilt updated to 2.56 with corresponding Compose integration to 1.2.0
- Payment library (Razorpay) updated to 1.6.41
4-4: Verify Kotlin 2.2.0 release and ecosystem compatibility
- Confirmed that JetBrains has officially published Kotlin v2.2.0 (GitHub tag “v2.2.0”).
- Please ensure that all Kotlin Gradle plugins (kotlin-gradle-plugin, kapt, serialization, coroutines, etc.) and any third-party Kotlin libraries in your build are updated to versions compatible with Kotlin 2.2.0.
- Run a full local and CI build (including any annotation-processing and code-generation steps) to surface any breaking changes before merging.
| - name: Set up JDK | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '21' | ||
|
|
||
| - name: Setup Gradle | ||
| uses: gradle/actions/setup-gradle@v4 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Android builds typically require SDK installation; add setup-android step if AGP modules are present.
The project appears to include Android modules (Compose app). Without an SDK, ./gradlew build often fails on CI.
First, confirm AGP usage:
If present, add before Build:
- name: Set up JDK
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: '21'
+ - name: Set up Android SDK
+ uses: android-actions/setup-android@v3
+ with:
+ api-level: 34
+ ndk: 26.1.10909125
- name: Setup Gradle
uses: gradle/actions/setup-gradle@v4🏁 Script executed:
#!/bin/bash
# Detect Android Gradle Plugin usage and required compileSdk.
rg -nP -C2 'com\.android\.(application|library)|compileSdk|namespace' --type=gradle.ktsLength of output: 124
🏁 Script executed:
#!/bin/bash
# Search for Android Gradle Plugin usage in all Gradle scripts
rg -nP 'com\.android\.(application|library)|compileSdk|namespace' -g '*.gradle*' -n -C2Length of output: 3348
🏁 Script executed:
#!/bin/bash
# Locate ConfigData declaration
rg -n 'object ConfigData' -g '*.kt' -n -C2
# Find compileSdkVersion definition
rg -n 'compileSdkVersion' -g '*.kt' -n -C2Length of output: 656
Add Android SDK setup to CI workflow
This repository defines multiple Android modules (application and library plugins detected in build.gradle.kts) all targeting compileSdkVersion = 36 (see buildSrc/src/main/java/ConfigData.kt) and will fail to build on CI without the Android SDK installed. Insert an Android SDK setup step before the Gradle setup:
- name: Set up JDK
uses: actions/setup-java@v4
with:
distribution: temurin
java-version: '21'
+ - name: Set up Android SDK
+ uses: android-actions/setup-android@v3
+ with:
+ api-level: 36 # matches ConfigData.compileSdkVersion
+ components: # install required SDK components
+ - build-tools;36.0.0
+ - platform-tools
+ # ndk: <version> # add if you have native code
- name: Setup Gradle
uses: gradle/actions/setup-gradle@v4- api-level must match
ConfigData.compileSdkVersion(36) to satisfy the AGP modules. - Include any additional components (e.g., NDK) only if your modules require native builds.
📝 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.
| - name: Set up JDK | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: temurin | |
| java-version: '21' | |
| - name: Setup Gradle | |
| uses: gradle/actions/setup-gradle@v4 | |
| - name: Set up JDK | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: temurin | |
| java-version: '21' | |
| - name: Set up Android SDK | |
| uses: android-actions/setup-android@v3 | |
| with: | |
| api-level: 36 # matches ConfigData.compileSdkVersion | |
| components: # install required SDK components | |
| - build-tools;36.0.0 | |
| - platform-tools | |
| # ndk: <version> # add if you have native code | |
| - name: Setup Gradle | |
| uses: gradle/actions/setup-gradle@v4 |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 16 to 24: the CI workflow currently sets
up JDK and then Gradle but does not install the Android SDK, causing Android
modules targeting compileSdkVersion 36 to fail; add a step before the "Setup
Gradle" step that installs the Android SDK platform 36 and required components
(platform-tools, build-tools matching the platform, and any optional components
like NDK only if native builds are needed), accept licenses, and export
ANDROID_SDK_ROOT/ANDROID_HOME so Gradle/AGP can find the SDK; ensure the
api-level installed matches ConfigData.compileSdkVersion (36) and that the step
runs on the same runner environment as the rest of the job.
| @Suppress("UnstableApiUsage") | ||
| androidResources { | ||
| localeFilters.addAll(listOf("en", "hi", "iw")) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
androidResources.localeFilters placement and language code
- The
androidResources { localeFilters }block is insidedefaultConfig. AGP expectsandroidResourcesunder the top-levelandroid {}scope. Also, use “he” instead of the deprecated “iw”.
defaultConfig {
...
- @Suppress("UnstableApiUsage")
- androidResources {
- localeFilters.addAll(listOf("en", "hi", "iw"))
- }
}
+ @Suppress("UnstableApiUsage")
+ androidResources {
+ // Prefer modern BCP-47 tags; 'iw' (Hebrew) is deprecated -> use 'he'
+ localeFilters.addAll(listOf("en", "hi", "he"))
+ }Please confirm your AGP version supports localeFilters (AGP 8.3+). If not, fall back to defaultConfig { resourceConfigurations += listOf("en","hi","he") }.
🏁 Script executed:
#!/bin/bash
# AGP plugin version in buildscript dependencies
echo "=== AGP plugin version in buildscript dependencies ==="
grep -R "com.android.tools.build:gradle" -n .
echo ""
echo "=== AGP plugin version in plugins DSL ==="
grep -R 'id("com.android.application")' -n .
echo ""
echo "=== defaultConfig context ==="
grep -n -C5 'defaultConfig' app/build.gradle.kts
echo ""
echo "=== androidResources context ==="
grep -n -C5 'androidResources' app/build.gradle.ktsLength of output: 1623
🏁 Script executed:
#!/bin/bash
# Locate AGP plugin version constant in buildSrc
grep -R -n -C3 "buildGradle" buildSrcLength of output: 1074
Move androidResources block out of defaultConfig and update deprecated language tag
The project is using AGP 8.12.0 (≥ 8.3), so androidResources.localeFilters is supported at the top-level android {} scope—but not inside defaultConfig. Additionally, "iw" is the deprecated alias for Hebrew; use the modern BCP-47 tag "he" instead.
Please apply the following mandatory refactor in app/build.gradle.kts:
- Remove the
androidResources { … }block (lines 26–29) from insidedefaultConfig. - Add it under the top-level
android {}scope (e.g. immediately afterdefaultConfig { … }), updating"iw"→"he".
app/build.gradle.kts
android {
compileSdk = ConfigData.compileSdkVersion
defaultConfig {
applicationId = "bose.ankush.weatherify"
minSdk = ConfigData.minSdkVersion
targetSdk = ConfigData.targetSdkVersion
versionCode = ConfigData.versionCode
versionName = ConfigData.versionName
multiDexEnabled = ConfigData.multiDexEnabled
testInstrumentationRunner = "bose.ankush.weatherify.helper.HiltTestRunner"
- @Suppress("UnstableApiUsage")
- androidResources {
- localeFilters.addAll(listOf("en", "hi", "iw"))
- }
}
+ @Suppress("UnstableApiUsage")
+ androidResources {
+ // Use modern BCP-47 tags; 'he' replaces deprecated 'iw'
+ localeFilters.addAll(listOf("en", "hi", "he"))
+ }
kapt {
…📝 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.
| @Suppress("UnstableApiUsage") | |
| androidResources { | |
| localeFilters.addAll(listOf("en", "hi", "iw")) | |
| } | |
| android { | |
| compileSdk = ConfigData.compileSdkVersion | |
| defaultConfig { | |
| applicationId = "bose.ankush.weatherify" | |
| minSdk = ConfigData.minSdkVersion | |
| targetSdk = ConfigData.targetSdkVersion | |
| versionCode = ConfigData.versionCode | |
| versionName = ConfigData.versionName | |
| multiDexEnabled = ConfigData.multiDexEnabled | |
| testInstrumentationRunner = "bose.ankush.weatherify.helper.HiltTestRunner" | |
| } | |
| @Suppress("UnstableApiUsage") | |
| androidResources { | |
| // Use modern BCP-47 tags; 'he' replaces deprecated 'iw' | |
| localeFilters.addAll(listOf("en", "hi", "he")) | |
| } | |
| kapt { | |
| … | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/build.gradle.kts around lines 26–29, the androidResources { … } block is
incorrectly placed inside defaultConfig and uses the deprecated locale tag "iw";
remove the entire androidResources block from inside defaultConfig and add it at
the top-level android { } scope (for example immediately after the closing brace
of defaultConfig), preserving the @Suppress("UnstableApiUsage") if needed and
updating the locale list to use "he" instead of "iw" (e.g.
localeFilters.addAll(listOf("en", "hi", "he"))).
| implementation(Deps.room) | ||
| implementation(Deps.roomKtx) | ||
|
|
||
| // Firebase |
There was a problem hiding this comment.
Room compiler missing — annotation processing will fail at build time
You’ve added Room runtime but not the processor. Without room-compiler, DAOs/entities won’t generate. Add the kapt (or KSP) dependency.
dependencies {
@@
- // Room runtime for providing WeatherDatabase from app DI
- implementation(Deps.room)
- implementation(Deps.roomKtx)
+ // Room
+ implementation(Deps.room)
+ implementation(Deps.roomKtx)
+ kapt(Deps.roomCompiler)
}If you’re migrating to KSP, replace kapt with ksp(Deps.roomCompiler) and apply the KSP plugin instead of kapt.
🤖 Prompt for AI Agents
In app/build.gradle.kts around lines 127 to 130, you added Room runtime
(implementation(Deps.room) and implementation(Deps.roomKtx)) but omitted the
Room annotation processor; this will cause DAO/entity generation to fail at
build time. Add the Room compiler as an annotation processor (either
kapt(Deps.roomCompiler) and apply the kapt plugin in the Gradle file, or if
using KSP use ksp(Deps.roomCompiler) and apply the KSP plugin), ensuring the
chosen processor configuration matches the rest of the project (kapt vs ksp) and
remove any conflicting processor config.
| class PreferenceManagerImpl @Inject constructor(@get:ApplicationContext private val context: Context) : | ||
| PreferenceManager { | ||
|
|
There was a problem hiding this comment.
Incorrect use-site target for @ApplicationContext may break Hilt injection.
Use @ApplicationContext directly (or @param:) on the constructor parameter. @get:ApplicationContext annotates the getter, not the injected parameter.
Apply:
-@Singleton
-class PreferenceManagerImpl @Inject constructor(@get:ApplicationContext private val context: Context) :
- PreferenceManager {
+@Singleton
+class PreferenceManagerImpl @Inject constructor(@ApplicationContext private val context: Context) :
+ PreferenceManager {Alternatively:
class PreferenceManagerImpl @Inject constructor(
@param:ApplicationContext private val context: Context
) : PreferenceManager🤖 Prompt for AI Agents
In app/src/main/java/bose/ankush/weatherify/data/preference/PreferenceManager.kt
around lines 19-21, the @get:ApplicationContext use-site target is wrong and
will not annotate the constructor parameter for Hilt; replace it by applying
@ApplicationContext directly to the constructor parameter (or use
@param:ApplicationContext) so Hilt injects the application Context into the
constructor parameter, keeping the rest of the class signature intact.
| class TokenStorageImpl( | ||
| private val database: WeatherDatabase | ||
| ) : TokenStorage { | ||
|
|
||
| override suspend fun saveToken(token: String) { | ||
| withContext(Dispatchers.IO) { | ||
| val authToken = AuthToken(token = token) | ||
| database.authTokenDao().saveToken(authToken) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun getToken(): String? { | ||
| return withContext(Dispatchers.IO) { | ||
| database.authTokenDao().getToken()?.token | ||
| } | ||
| } | ||
|
|
||
| override fun hasToken(): Flow<Boolean> { | ||
| // Flow is already asynchronous, so we don't need to use withContext here | ||
| return database.authTokenDao().hasToken() | ||
| } | ||
|
|
||
| override suspend fun clearToken() { | ||
| withContext(Dispatchers.IO) { | ||
| database.authTokenDao().clearTokens() | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Token stored in plaintext Room table — encrypt at rest with Android Keystore
Access tokens are sensitive. Storing them as plain text in Room risks leakage (rooted device, backups, debug DB dumps). Encrypt before persisting and decrypt on read, with keys in Android Keystore.
Proposed approach (high level):
- Introduce a small SecureTokenCodec that uses Android Keystore (AES-GCM with a Keystore-wrapped key).
- Encode ciphertext+IV as Base64 for storage in the same column.
- Swap encode/decode in saveToken/getToken.
I can add SecureTokenCodec and wire it here with unit tests using Robolectric/Instrumented tests.
🤖 Prompt for AI Agents
In storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt
around lines 13–40, the token is currently persisted in plaintext; replace
persistence with encryption by wiring a SecureTokenCodec that uses Android
Keystore-backed AES-GCM (generate or load a symmetric key wrapped by Keystore),
have saveToken call codec.encode(token) and store the resulting Base64
ciphertext+IV string in the same column, have getToken call
codec.decode(storedBase64) to return plaintext; keep all DB calls on
Dispatchers.IO, handle and surface decryption failures (return null or clear
token) and ensure the codec is thread-safe and unit/instrumentation-testable
(use Robolectric/AndroidTest for key material).
| override suspend fun saveToken(token: String) { | ||
| withContext(Dispatchers.IO) { | ||
| val authToken = AuthToken(token = token) | ||
| database.authTokenDao().saveToken(authToken) | ||
| } | ||
| } |
There was a problem hiding this comment.
Guarantee “single-row” token semantics and atomicity
As written, multiple rows can accrue unless the DAO enforces uniqueness. Make the write atomic and replace existing token to prevent ambiguity for getToken()/hasToken().
Apply this diff (requires room-ktx, already present):
+import androidx.room.withTransaction
@@
override suspend fun saveToken(token: String) {
withContext(Dispatchers.IO) {
- val authToken = AuthToken(token = token)
- database.authTokenDao().saveToken(authToken)
+ val authToken = AuthToken(token = token)
+ database.withTransaction {
+ // Ensure only one token row exists
+ database.authTokenDao().clearTokens()
+ database.authTokenDao().saveToken(authToken)
+ }
}
}If you already have an upsert in AuthTokenDao, call that instead inside withTransaction.
🤖 Prompt for AI Agents
In storage/src/androidMain/kotlin/bose/ankush/storage/impl/TokenStorageImpl.kt
around lines 17 to 22, the current saveToken implementation writes a new
AuthToken row without guaranteeing single-row semantics or atomic replace,
allowing multiple rows to accrue; update the implementation to perform the write
inside a database.withTransaction and either call the DAO's upsert method (if
present) or perform a deleteAll then insert within the same transaction so the
token replace is atomic and getToken()/hasToken() remain unambiguous.
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 24.dp, vertical = 16.dp), | ||
| horizontalAlignment = Alignment.CenterHorizontally | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the sheet scrollable to avoid clipping on small screens.
Wrap main content with verticalScroll to ensure accessibility on compact heights:
- Column(
- modifier = Modifier
- .fillMaxWidth()
- .padding(horizontal = 24.dp, vertical = 16.dp),
+ Column(
+ modifier = Modifier
+ .fillMaxWidth()
+ .verticalScroll(rememberScrollState())
+ .padding(horizontal = 24.dp, vertical = 16.dp),📝 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.
| Column( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .padding(horizontal = 24.dp, vertical = 16.dp), | |
| horizontalAlignment = Alignment.CenterHorizontally | |
| ) { | |
| Column( | |
| modifier = Modifier | |
| .fillMaxWidth() | |
| .verticalScroll(rememberScrollState()) | |
| .padding(horizontal = 24.dp, vertical = 16.dp), | |
| horizontalAlignment = Alignment.CenterHorizontally | |
| ) { |
🤖 Prompt for AI Agents
In
sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt
around lines 42 to 47, the Column's content can be clipped on small screens
because it isn't scrollable; wrap the main Column with
Modifier.verticalScroll(rememberScrollState()) (importing rememberScrollState
and verticalScroll) by adding .verticalScroll(rememberScrollState()) to the
modifier chain (e.g.,
Modifier.fillMaxWidth().verticalScroll(rememberScrollState()).padding(...)) so
the sheet becomes vertically scrollable and content isn't clipped on compact
heights.
| SimplePremiumFeature("Ad-Free Experience") | ||
| SimplePremiumFeature("Extended 15-day Forecasts") | ||
| SimplePremiumFeature("Severe Weather Alerts") | ||
| SimplePremiumFeature("Detailed Air Quality Data") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coded strings/prices; externalize and/or parameterize.
- The feature labels and pricing text are hard-coded, bypassing your localization layer.
- Consider passing
features: List<String>andpriceText: String, or using string resources.
- SimplePremiumFeature("Ad-Free Experience")
- SimplePremiumFeature("Extended 15-day Forecasts")
- SimplePremiumFeature("Severe Weather Alerts")
- SimplePremiumFeature("Detailed Air Quality Data")
+ features.forEach { SimplePremiumFeature(it) }
- Text(text = "$4.99/month", ...)
- Text(text = "7-day free trial, cancel anytime", ...)
+ Text(text = priceText, ...)
+ Text(text = trialDisclaimer, ...)Also applies to: 79-91
🤖 Prompt for AI Agents
In
sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt
around lines 69-74 (and similarly 79-91), feature labels and price text are
hard-coded; change the composable to accept features: List<String> and
priceText: String (or use @StringRes resource IDs) and replace literal strings
with those parameters or stringResource lookups so all UI text is externalized
for localization; update call sites to pass the feature list and price text (or
resource IDs) instead of relying on hard-coded values.
| // Subscribe Button | ||
| val isStarting = remember { mutableStateOf(false) } | ||
| Button( | ||
| onClick = { | ||
| if (!isStarting.value) { | ||
| isStarting.value = true | ||
| onSubscribe() | ||
| } | ||
| }, | ||
| enabled = !isStarting.value, | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(48.dp), | ||
| colors = ButtonDefaults.buttonColors( | ||
| containerColor = Color(0xFFFFB74D) | ||
| ), | ||
| shape = RoundedCornerShape(8.dp) | ||
| ) { | ||
| if (isStarting.value) { | ||
| Row(verticalAlignment = Alignment.CenterVertically) { | ||
| CircularProgressIndicator( | ||
| modifier = Modifier.size(18.dp), | ||
| color = Color.White, | ||
| strokeWidth = 2.dp | ||
| ) | ||
| Spacer(modifier = Modifier.width(12.dp)) | ||
| Text( | ||
| text = "Starting...", | ||
| style = MaterialTheme.typography.titleMedium, | ||
| fontWeight = FontWeight.Medium, | ||
| color = Color.White | ||
| ) | ||
| } | ||
| } else { | ||
| Text( | ||
| text = "Subscribe", | ||
| style = MaterialTheme.typography.titleMedium, | ||
| fontWeight = FontWeight.Medium, | ||
| color = Color.White | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Loading state should be driven from the caller; provide a parameter instead of internal sticky state.
isStarting sticks “true” indefinitely if onSubscribe() doesn’t dismiss the sheet, and it obscures ViewModel-driven state. Pass an isLoading: Boolean and render accordingly; let the caller own transitions.
-fun PremiumBottomSheetContent(
- onDismiss: () -> Unit,
- onSubscribe: () -> Unit
-) {
+fun PremiumBottomSheetContent(
+ onDismiss: () -> Unit,
+ onSubscribe: () -> Unit,
+ isLoading: Boolean = false
+) {
...
- val isStarting = remember { mutableStateOf(false) }
Button(
- onClick = {
- if (!isStarting.value) {
- isStarting.value = true
- onSubscribe()
- }
- },
- enabled = !isStarting.value,
+ onClick = onSubscribe,
+ enabled = !isLoading,
...
) {
- if (isStarting.value) {
+ if (isLoading) {
...
} else {
...
}
}Lift PremiumBottomSheetContent into a stateless composable by externalizing its loading state
To prevent the button’s “Starting…” state from becoming permanently stuck and to align with Compose best practices (stateless composables driven entirely by their inputs), refactor PremiumBottomSheetContent as follows:
• Add a new parameter isLoading: Boolean (defaulting to false) and remove the internal remember { mutableStateOf(false) }.
• Change the button’s onClick to simply call onSubscribe() and its enabled/content logic to depend on isLoading.
• Push responsibility for injecting and toggling that loading flag up to the caller (e.g., your ViewModel).
fun PremiumBottomSheetContent(
onDismiss: () -> Unit,
onSubscribe: () -> Unit,
- // internal, sticky state—remove this
- // val isStarting = remember { mutableStateOf(false) }
+ // lifted state
+ isLoading: Boolean = false
) {
Button(
- onClick = {
- if (!isStarting.value) {
- isStarting.value = true
- onSubscribe()
- }
- },
- enabled = !isStarting.value,
+ onClick = onSubscribe,
+ enabled = !isLoading,
modifier = Modifier
.fillMaxWidth()
.height(48.dp),
colors = ButtonDefaults.buttonColors(containerColor = Color(0xFFFFB74D)),
shape = RoundedCornerShape(8.dp)
) {
- if (isStarting.value) {
+ if (isLoading) {
Row(verticalAlignment = Alignment.CenterVertically) {
CircularProgressIndicator(
modifier = Modifier.size(18.dp),
color = Color.White,
strokeWidth = 2.dp
)
Spacer(modifier = Modifier.width(12.dp))
Text(
text = "Starting...",
style = MaterialTheme.typography.titleMedium,
fontWeight = FontWeight.Medium,
color = Color.White
)
}
} else {
Text(
text = "Subscribe",
style = MaterialTheme.typography.titleMedium,
fontWeight = FontWeight.Medium,
color = Color.White
)
}
}
}• Caller adjustments
– Pass your ViewModel’s loading state into PremiumBottomSheetContent(isLoading = viewModel.isSubscribing)
– Trigger viewModel.isSubscribing = true in onSubscribe() and reset it when the subscription flow completes or fails.
This refactor ensures that UI state stays in sync with your app’s business logic and avoids “sticky” local state.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt
around lines 95 to 136, the composable currently holds internal mutable state
(isStarting) which can become "stuck"; make the composable stateless by removing
remember { mutableStateOf(false) }, add a Boolean parameter isLoading = false,
change the Button onClick to only call onSubscribe(), and drive enabled/content
(show "Starting..." vs "Subscribe" and enable flag) from isLoading; update
callers to pass the ViewModel's loading state and let the ViewModel toggle that
flag during the subscription flow.
This commit introduces a major redesign of the Settings screen for a cleaner, more organized user experience. It also adds a new feedback submission feature, improves the robustness of the payment flow, and includes various stability and code quality enhancements.
- **UI Overhaul (SettingsScreen):**
- **Complete Redesign:** Replaced the previous card-based layout with a modern, sectioned list design using `LazyColumn`. The new layout is cleaner and more aligned with platform conventions.
- **New Components:**
- Introduced `SettingsSection`, `LegalSection`, and `SettingsItem` composables for a structured and reusable UI.
- `PremiumCard`: A new, prominent card at the top dynamically displays "Get Premium" or "Premium Active" states with a refined design.
- The screen now features a `CenterAlignedTopAppBar` with a back button for standard navigation.
- **Staggered Animations:** Implemented `slideInVertically` and `fadeIn` animations for screen elements to enhance visual appeal.
- **Conditional UI:** The "Notifications" settings item is now only displayed on Android 13 (Tiramisu) and above if the permission has not been granted.
- **Legal & Info:** Privacy Policy and Terms of Use now open in a browser. App version is displayed in the legal section.
- **Feat: Feedback Submission:**
- **API Integration:**
- Added `FeedbackApiService`, `FeedbackRepository`, and their Ktor/Impl versions to the `network` module for submitting user feedback.
- The `FeedbackRepository` is now available via Hilt DI (`createFeedbackRepository`).
- **Data Models:** Introduced `FeedbackRequest` and `FeedbackResponse` models.
- **Domain Layer:** Added a `SubmitFeedback` use case in the `app` module.
- **Refactor: Payment Flow & Error Handling:**
- **Result-based Repository:** `PaymentRepository` methods (`createOrder`, `verifyPayment`) now return `Result<T>` instead of throwing exceptions directly. This improves error handling and clarifies success/failure paths.
- **ViewModel Update:** `MainViewModel` is updated to handle the `Result` from payment use cases using `fold`, providing clearer error state management for the UI.
- **Use Cases:** New `CreateOrder` and `VerifyPayment` use cases now wrap the repository calls and return a `Result`.
- **Code Quality & Stability:**
- **Enhanced Logging:** `Timber` is now configured to only log `WARN`, `ERROR`, and `WTF` priorities in release builds, reducing log verbosity.
- **Location Accuracy:** Switched from `client.lastLocation` to `client.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY)` to request a fresh, more accurate location instead of a potentially stale cached one.
- **Android Manifest:**
- Disabled cleartext traffic (`usesCleartextTraffic="false"`) for improved security.
- Replaced the deprecated `CALL_PHONE` permission with `FOREGROUND_SERVICE_LOCATION`.
- **Dependency Updates:** Added `androidx.compose.material:material-icons-extended` to `app` and `sunriseui` modules.
- **Typography:** Updated `AppTypography` to use a bundled `Inter` font family for a consistent, modern look, replacing `FontFamily.Default`.
- **Build & DI:** Removed a trailing slash from `WEATHER_BASE_URL`. Moved the `StorageModule.kt` from the `storage` module to the `app` module to decouple `storage` from Hilt dependencies.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/bose/ankush/weatherify/base/location/DeviceLocationClient.kt (1)
105-107: Update outdated comment.The comment references
lastLocation, but the code now usesgetCurrentLocation.Apply this diff:
continuation.invokeOnCancellation { - // No need to cancel anything for lastLocation as it's a one-time operation + // Location request cancellation should be handled here (see above suggestion) }
♻️ Duplicate comments (5)
app/build.gradle.kts (2)
26-29: MoveandroidResourcesout ofdefaultConfigand update locale tag
androidResources { … }underdefaultConfigisn’t recognized by AGP; the block needs to live directly insideandroid {}. While fixing the placement, swap the deprecated"iw"tag for the modern"he"variant. A quick patch:- @Suppress("UnstableApiUsage") - androidResources { - localeFilters.addAll(listOf("en", "hi", "iw")) - } } + @Suppress("UnstableApiUsage") + androidResources { + localeFilters.addAll(listOf("en", "hi", "he")) + }
128-131: Room compiler is still missing
implementation(Deps.room)/roomKtxalone won’t generate DAOs and entities—kapt(Deps.roomCompiler)(or the KSP equivalent) is required, otherwise builds fail at annotation processing. Please add the processor alongside the existing Room deps.implementation(Deps.room) implementation(Deps.roomKtx) + kapt(Deps.roomCompiler)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (2)
171-173: Guard against empty permission queueCalling
permissionDialogQueue.removeAt(0)will crash when the queue is already empty (e.g., dialog dismissed twice or cleared elsewhere). Add an emptiness check before removing.- fun dismissDialog() = permissionDialogQueue.removeAt(0) + fun dismissDialog() { + if (permissionDialogQueue.isNotEmpty()) { + permissionDialogQueue.removeAt(0) + } + }
175-179: Avoid fetching location before all permissions are grantedBecause
fetchAndSaveLocationCoordinates()runs in theelsebranch, any single granted permission triggers a location fetch even if other required permissions were denied or are still pending. This can lead to repeated work and failures when the location client lacks the full permission set. Please gate the fetch on an aggregate permission check (e.g., ensure all required location permissions are granted via your permissions helper) before callingfetchAndSaveLocationCoordinates().network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (1)
48-52: Stop reusing platform client engines via throwaway HttpClient instances
HttpClient(createPlatformHttpClient(json).engine)spins up a platform client, immediately abandons it (never closed), and recreates a new client without the platform-installed plugins/timeouts. This is the same leak/misconfiguration called out earlier. Please obtain the platform client once and reuse it (e.g.,val platform = createPlatformHttpClient(json); return platform.config { configureAuth(...) /* etc. */ }), or simply return the platform client when no extra configuration is needed.Also applies to: 123-138, 156-170
🧹 Nitpick comments (4)
app/src/main/java/bose/ankush/weatherify/base/location/DeviceLocationClient.kt (1)
93-93: Consider providing a CancellationToken for proper cancellation.Switching from
lastLocationtogetCurrentLocationactively requests a fresh location fix (more accurate but slower). However, passingnullfor theCancellationTokenparameter prevents cancelling the location request if the coroutine is cancelled. This could lead to wasted battery and unnecessary location requests continuing after the caller no longer needs the result.Consider creating a
CancellationTokenSourceand wiring it tocontinuation.invokeOnCancellation:+import com.google.android.gms.tasks.CancellationTokenSource + @SuppressLint("MissingPermission") override suspend fun getCurrentLocation(): Result<Location> = suspendCancellableCoroutine { continuation -> try { checkLocationPermission() checkGpsEnabled() } catch (e: LocationClient.LocationException) { continuation.resume(Result.failure(e)) return@suspendCancellableCoroutine } + val cancellationTokenSource = CancellationTokenSource() - client.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, null) + client.getCurrentLocation(Priority.PRIORITY_HIGH_ACCURACY, cancellationTokenSource.token) .addOnSuccessListener { location -> if (location != null) { continuation.resume(Result.success(location)) } else { continuation.resume(Result.failure(LocationClient.LocationException("Location is null"))) } } .addOnFailureListener { e -> continuation.resume(Result.failure(LocationClient.LocationException(e.message ?: "Unknown error"))) } continuation.invokeOnCancellation { - // No need to cancel anything for lastLocation as it's a one-time operation + cancellationTokenSource.cancel() } }app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (2)
391-391: Use theme color instead of hardcoded value.The hardcoded green
Color(0xFF2E7D32)breaks theme consistency. Consider using a semantic color from MaterialTheme.Apply this diff:
- color = Color(0xFF2E7D32), + color = MaterialTheme.colorScheme.primary,or if you need a specific green shade:
- color = Color(0xFF2E7D32), + color = MaterialTheme.colorScheme.tertiary,
451-456: Consider extracting URLs to constants.The hardcoded privacy policy and terms URLs could be extracted to a constants file for easier maintenance and potential reuse.
For example, in a new
AppConstants.kt:object AppConstants { const val PRIVACY_POLICY_URL = "https://data.androidplay.in/wfy/privacy-policy" const val TERMS_URL = "https://data.androidplay.in/wfy/terms-and-conditions" }Then use:
- onClick = { context.openUrlInBrowser("https://data.androidplay.in/wfy/privacy-policy") } + onClick = { context.openUrlInBrowser(AppConstants.PRIVACY_POLICY_URL) }app/src/main/java/bose/ankush/weatherify/WeatherifyApplication.kt (1)
35-46: Consider adding a comment explaining the log level filtering.Adding a brief comment explaining why certain log levels are suppressed in release builds (e.g., security concerns, performance, log noise reduction) would improve code maintainability.
Example:
private fun enableTimber() { if (BuildConfig.DEBUG) { Timber.plant(Timber.DebugTree()) } else { + // In production, suppress verbose/debug/info logs to reduce log noise, + // improve performance, and avoid leaking sensitive information Timber.plant(object : Timber.Tree() { override fun log(priority: Int, tag: String?, message: String, t: Throwable?) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/build.gradle.kts(6 hunks)app/src/main/AndroidManifest.xml(2 hunks)app/src/main/java/bose/ankush/storage/di/StorageModule.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/WeatherifyApplication.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/base/location/DeviceLocationClient.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt(2 hunks)app/src/main/java/bose/ankush/weatherify/domain/use_case/feedback/SubmitFeedback.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/domain/use_case/payment/CreateOrder.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/domain/use_case/payment/VerifyPayment.kt(1 hunks)app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt(3 hunks)app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt(4 hunks)app/src/main/java/bose/ankush/weatherify/presentation/theme/Type.kt(1 hunks)buildSrc/src/main/java/Dependencies.kt(3 hunks)network/src/commonMain/kotlin/bose/ankush/network/api/FeedbackApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/api/KtorFeedbackApiService.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt(2 hunks)network/src/commonMain/kotlin/bose/ankush/network/model/FeedbackModels.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/FeedbackRepository.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/FeedbackRepositoryImpl.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepository.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt(1 hunks)network/src/commonMain/kotlin/bose/ankush/network/utils/Constants.kt(1 hunks)sunriseui/build.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- buildSrc/src/main/java/Dependencies.kt
- app/src/main/java/bose/ankush/weatherify/di/NetworkModule.kt
- network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt
- sunriseui/build.gradle.kts
🧰 Additional context used
🧬 Code graph analysis (4)
app/build.gradle.kts (1)
buildSrc/src/main/kotlin/KmmDeps.kt (1)
implementation(59-61)
app/src/main/java/bose/ankush/weatherify/presentation/MainViewModel.kt (8)
app/src/main/java/bose/ankush/weatherify/base/common/UiText.kt (1)
errorResponseFromException(50-55)storage/src/androidMain/kotlin/bose/ankush/storage/impl/WeatherStorageImpl.kt (1)
getWeatherReport(35-37)app/src/main/java/bose/ankush/weatherify/data/repository/WeatherRepositoryImpl.kt (1)
getWeatherReport(31-34)app/src/main/java/bose/ankush/weatherify/domain/repository/WeatherRepository.kt (1)
getWeatherReport(16-16)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepository.kt (3)
createOrder(11-23)createOrder(16-16)verifyPayment(22-22)network/src/commonMain/kotlin/bose/ankush/network/repository/PaymentRepositoryImpl.kt (2)
createOrder(18-23)verifyPayment(25-30)network/src/commonMain/kotlin/bose/ankush/network/api/KtorPaymentApiService.kt (2)
createOrder(23-30)verifyPayment(32-39)network/src/commonMain/kotlin/bose/ankush/network/api/PaymentApiService.kt (3)
createOrder(11-21)createOrder(15-15)verifyPayment(20-20)
network/src/commonMain/kotlin/bose/ankush/network/di/NetworkModule.kt (3)
network/src/androidMain/kotlin/bose/ankush/network/di/AndroidHttpClient.kt (1)
createPlatformHttpClient(16-35)network/src/iosMain/kotlin/bose/ankush/network/di/IOSHttpClient.kt (1)
createPlatformHttpClient(15-36)network/src/commonMain/kotlin/bose/ankush/network/auth/interceptor/AuthInterceptor.kt (2)
configureAuth(20-73)configureAuth(79-88)
app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (1)
sunriseui/src/main/java/bose/ankush/sunriseui/premium/PremiumBottomSheetContent.kt (1)
PremiumBottomSheetContent(37-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: qodana
- GitHub Check: build
🔇 Additional comments (19)
app/src/main/AndroidManifest.xml (1)
25-25: Double-check HTTP dependencies before disabling cleartext.Setting
android:usesCleartextTraffic="false"blocks every plain-HTTP request (including WebView content and any third-party SDKs). Please confirm that every endpoint—production, staging, and any feature toggles—already uses HTTPS or declare an explicit network security config exception; otherwise this change will break those flows.If you need help auditing the codebase for
http://usages, let me know and I can script the search.app/src/main/java/bose/ankush/weatherify/base/location/DeviceLocationClient.kt (1)
22-22: LGTM: Import addition.The explicit import of
kotlin.coroutines.resumealigns with its usage throughout the file.app/src/main/java/bose/ankush/storage/di/StorageModule.kt (2)
64-70: TokenStorage binding added—past issue resolved.The active
@Providesfunction forTokenStoragedirectly addresses the critical Hilt binding issue flagged in the previous review. This should prevent the "no binding found forTokenStorage" runtime error.
51-51: Remove the boolean argument fromfallbackToDestructiveMigration
The standard Room API only provides a no-argfallbackToDestructiveMigration(). Passingfalseisn’t supported and will fail to compile unless you’ve defined a custom extension. Remove the parameter or confirm that your Room version or a custom API actually supports a boolean overload.app/src/main/java/bose/ankush/weatherify/presentation/settings/SettingsScreen.kt (6)
3-79: LGTM! Clean import organization.The new imports support the enhanced functionality: runtime permissions, Material3 components, authentication state, payment UI, and date formatting. All imports are utilized in the code.
81-286: Excellent refactoring of SettingsScreen.The migration from inline UI to modular composables (PremiumCard, SettingsSection, LegalSection, SettingsItem) significantly improves maintainability. Key improvements:
- LazyColumn for better scrolling performance
- Staggered AnimatedVisibility for polished entrance animations
- Proper auth state observation for logout flow
- CenterAlignedTopAppBar aligns with Material3 guidelines
The logout dialog correctly disables actions during logout and resets auth state after success.
288-396: Premium card composables are well-structured.The conditional rendering based on subscription status and the use of
.then()for conditional clickable modifier (line 304) are good patterns. The expiry date formatting (lines 378-386) correctly handles optional expiry display.
398-436: Proper runtime permission handling.The notification item visibility logic (lines 406-414) correctly handles API level differences:
- Shows notification settings only on API 33+ when permission is not granted
- Hides the item on older APIs where runtime permission isn't required
This provides a clean UX by showing the setting only when user action is needed.
438-470: LegalSection provides clean access to app info.Using
BuildConfig.VERSION_NAME(line 463) for the app version display and theopenUrlInBrowserextension for legal links are good practices.
473-513: Excellent reusable SettingsItem composable.Well-designed API with optional parameters for flexibility:
onClickmakes items conditionally interactivetrailingContentallows custom trailing UI (like version text)- Arrow icon automatically shown for clickable items without custom trailing content
This composable promotes consistency and reduces code duplication.
app/src/main/java/bose/ankush/weatherify/WeatherifyApplication.kt (1)
55-55: LGTM!Using
NOTIFICATION_SERVICEdirectly is correct and cleaner than the previousContext.NOTIFICATION_SERVICEapproach.network/src/commonMain/kotlin/bose/ankush/network/repository/FeedbackRepository.kt (1)
1-15: LGTM!The interface follows clean architecture principles with proper separation of concerns. The
Resultwrapper provides type-safe error handling, and the documentation clearly describes the contract.network/src/commonMain/kotlin/bose/ankush/network/api/KtorFeedbackApiService.kt (2)
23-23: Ensure endpoint path has leading slash for correct URL construction.With the recent removal of the trailing slash from
WEATHER_BASE_URLin Constants.kt (line 10), verify that the endpoint construction"$baseUrl/feedback"produces the correct URL. IfbaseUrlis"https://data.androidplay.in", this will correctly produce"https://data.androidplay.in/feedback".This is consistent with the pattern, but verify that all baseUrl usages across API services follow this pattern to avoid double slashes or missing slashes.
21-28: LGTM!The implementation follows best practices:
- Retry logic with exponential backoff handles transient failures
- Proper JSON content type specification
- Clean use of Ktor's DSL for HTTP requests
network/src/commonMain/kotlin/bose/ankush/network/api/FeedbackApiService.kt (1)
1-14: LGTM!Clean interface definition with clear documentation. The suspend function signature enables proper coroutine-based async handling.
app/src/main/java/bose/ankush/weatherify/domain/use_case/payment/VerifyPayment.kt (1)
1-13: LGTM!The use case follows the established pattern in the codebase:
- Simple delegation to the repository layer
Resultwrapper for type-safe error handling- Operator
invokefor clean call syntax- Proper dependency injection setup
Consistent with the
CreateOrderuse case structure.app/src/main/java/bose/ankush/weatherify/domain/use_case/payment/CreateOrder.kt (1)
1-13: LGTM!The use case implementation is clean and follows domain layer best practices:
- Thin wrapper around repository for single responsibility
- Proper DI integration
- Type-safe error handling with
ResultThe pattern is consistent across the payment use cases in this PR.
app/src/main/java/bose/ankush/weatherify/domain/use_case/feedback/SubmitFeedback.kt (1)
1-13: LGTM!Excellent consistency across all use cases in this PR. The implementation follows the established pattern:
- Single responsibility: delegates to repository
- Clean operator
invokesyntax- Proper dependency injection
- Type-safe
ResulthandlingAll three use cases (
CreateOrder,VerifyPayment,SubmitFeedback) follow this uniform approach, which improves maintainability.network/src/commonMain/kotlin/bose/ankush/network/utils/Constants.kt (1)
10-10: URL path construction correct All API calls use"$baseUrl/endpoint"with a leading slash; removing the trailing slash fromWEATHER_BASE_URLis safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (1)
47-47: Unnecessary string split operation.The expression
.split(" at").firstOrNull() ?: ""appears to serve no purpose. Based on the relevant code snippet,getAQIAnalysedTextreturns strings like "Air quality is Good" or "Air quality is fair", which do not contain " at". This split will never trigger and just adds cognitive overhead.Consider simplifying to:
- statusText = fullStatusText.split(" at").firstOrNull() ?: "", + statusText = fullStatusText,If the split was intended to handle future formats, add a comment explaining the expected format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (1)
app/src/main/java/bose/ankush/weatherify/base/common/AirQualityIndexAnalyser.kt (1)
getAQIAnalysedText(9-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: qodana
🔇 Additional comments (4)
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt (4)
63-89: LGTM: Well-structured card layout.The card layout refinements create a cleaner, more modern appearance with appropriate spacing, dividers, and elevation. The composition follows Material Design 3 principles effectively.
91-128: LGTM: Clean AQI summary implementation.The AqiSummary composable effectively presents the AQI information with good visual hierarchy and proper use of
contentColorForto ensure readable contrast between the background color and text.
170-188: LGTM: Clean pollutant item implementation.The PollutantItem composable is well-structured with appropriate typography hierarchy and alignment. The updated styles (titleMedium for values, bodySmall for labels) provide good visual distinction.
137-143: The unit label concern is invalid; the OpenWeatherMap Air Pollution API returns CO, O3, and PM2.5 all in μg/m³.According to OpenWeatherMap's API documentation, all air quality pollutant concentrations (including CO) are provided in micrograms per cubic meter (μg/m³), not in mg/m³ or ppm. The UI correctly displays all three pollutants with the same unit label, which aligns with the API specification. The code as written is accurate.
The observation about
Intconversion discarding precision (lines 137-139) remains a valid consideration if you want higher fidelity display values, but this is a minor UI presentation choice rather than a data correctness issue.Likely an incorrect or invalid review comment.
| private fun rememberAqiUiState(aqi: Int): AqiUiState { | ||
| return remember(aqi) { | ||
| val (fullStatusText, _) = getAQIAnalysedText(aqi) | ||
| AqiUiState( | ||
| statusText = fullStatusText.split(" at").firstOrNull() ?: "", | ||
| qualityColor = getAirQualityColor(aqi), | ||
| formattedAqi = aqi.getFormattedAQI() | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: AQI scale mismatch will cause incorrect color display.
The same aqi value is passed to both getAQIAnalysedText (line 45) and getAirQualityColor (line 48), but these functions expect different scales:
getAQIAnalysedTextexpects values 1-6 (as shown in the relevant code snippet)getAirQualityColor(lines 156-165) expects values 0-300+ (US EPA AQI scale)
One AQI value cannot be valid for both scales. This will result in incorrect color mapping—for example, an AQI of 3 (moderate on the 1-6 scale) would map to the "Good" green color on the 0-300+ scale.
Verify which AQI scale the AirQuality.aqi field uses and ensure consistent mapping:
🏁 Script executed:
#!/bin/bash
# Check the AirQuality model definition to determine the AQI scale
echo "=== Checking AirQuality model definition ==="
rg -n -A5 -B5 'class AirQuality|data class AirQuality' --type=kotlin
echo ""
echo "=== Checking AQI field usage in network/API responses ==="
rg -n -C3 '"aqi"' --type=kotlin --type=json
echo ""
echo "=== Checking other usages of getAQIAnalysedText ==="
rg -n -C3 'getAQIAnalysedText' --type=kotlinLength of output: 8469
---
🏁 Script executed:
```shell
#!/bin/bash
# Check getAirQualityColor function definition and implementation
echo "=== Checking getAirQualityColor definition ==="
rg -n -A15 'fun getAirQualityColor' --type=kotlin
echo ""
echo "=== Checking what scale is documented or expected ==="
rg -n -B5 -A10 'getAirQualityColor' --type=kotlin | head -60
Length of output: 7089
Critical: AQI scale mismatch causes incorrect color mapping.
The same aqi value is passed to both getAQIAnalysedText (line 45) and getAirQualityColor (line 48), but they expect incompatible scales:
getAQIAnalysedTextexpects OpenWeatherMap's 1-6 scale (1=Good, 2=Fair, ..., 6=Very Poor)getAirQualityColorexpects US EPA's 0-300+ scale (0-50=Good, 51-100=Moderate, etc.)
The API data uses OpenWeatherMap's 1-6 scale (mock data: "aqi": 5). An AQI of 5 will be correctly labeled as "very poor" but incorrectly colored green (because 5 falls in the 0-50 "Good" range of the EPA scale).
Fix: Convert the OpenWeatherMap AQI (1-6) to EPA scale (0-500) before passing to getAirQualityColor, or create a dedicated color function that accepts the 1-6 scale.
🤖 Prompt for AI Agents
In
app/src/main/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt
around lines 43-52, the OpenWeatherMap AQI (1-6) is passed directly to
getAirQualityColor which expects EPA-style 0-500 values; convert the 1-6 OWM
scale to an EPA-equivalent value before calling getAirQualityColor (or replace
the call with a dedicated color mapper that accepts 1-6). Implement a simple
mapping (e.g. 1 -> 25, 2 -> 75, 3 -> 125, 4 -> 175, 5 -> 300, 6 -> 425) or map
ranges (1=>0-50, 2=>51-100, etc.), use the converted EPA value in
getAirQualityColor, and keep getAQIAnalysedText(aqi) unchanged so labels remain
based on the original 1-6 scale.
...n/java/bose/ankush/weatherify/presentation/home/component/BriefAirQualityReportCardLayout.kt
Show resolved
Hide resolved
…tails and improved UI interactions
Issue
What was changed?
Summary by CodeRabbit
New Features
UI/UX
Permissions
Localization
Chores