-
Notifications
You must be signed in to change notification settings - Fork 1.3k
announcements #4314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
announcements #4314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive announcements feature, covering changelogs, new features, and general notices. The implementation is well-structured across both the backend and the mobile app, with clear separation of concerns. My review focuses on improving the robustness of the API client by ensuring proper URL encoding for all request parameters and completing the implementation of URL handling for call-to-action buttons in the UI, ensuring consistency with user expectations. These changes will prevent potential bugs and ensure the feature works as expected.
| var url = "${Env.apiBaseUrl}v1/announcements/features?version=$version&version_type=$versionType"; | ||
| if (deviceModel != null) { | ||
| url += "&device_model=$deviceModel"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query parameters version and deviceModel are not URL-encoded. This can lead to issues if they contain special characters (e.g., a + in a version string like 1.0.0+123 would be misinterpreted as a space). It's safer to use Uri.encodeComponent for all query parameter values, or construct the URI using its constructor with a queryParameters map to handle encoding automatically and robustly.
| var url = "${Env.apiBaseUrl}v1/announcements/features?version=$version&version_type=$versionType"; | |
| if (deviceModel != null) { | |
| url += "&device_model=$deviceModel"; | |
| } | |
| final queryParameters = { | |
| 'version': version, | |
| 'version_type': versionType, | |
| if (deviceModel != null) 'device_model': deviceModel, | |
| }; | |
| final url = Uri.parse('${Env.apiBaseUrl}v1/announcements/features').replace(queryParameters: queryParameters).toString(); |
| }) async { | ||
| var url = "${Env.apiBaseUrl}v1/announcements/general"; | ||
| if (excludeIds != null && excludeIds.isNotEmpty) { | ||
| url += "?exclude_ids=${excludeIds.join(',')}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclude_ids query parameter is not URL-encoded. While IDs are often URL-safe, it's best practice to encode all query parameter values to prevent potential issues if they ever contain special characters.
| url += "?exclude_ids=${excludeIds.join(',')}"; | |
| url += "?exclude_ids=${Uri.encodeComponent(excludeIds.join(','))}"; |
Three types of announcements, general/promotional, feature, changelog
Announcements can be created by admins from the admin dashboard
ScreenRecording_01-19-2026.19-25-17_1.MP4
ScreenRecording_01-20-2026.13-39-31_1.MP4