-
Notifications
You must be signed in to change notification settings - Fork 45
fix(cells): Time format doesn't follow the device settings (WPB-22171) #4502
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: develop
Are you sure you want to change the base?
Conversation
core/ui-common/src/main/kotlin/com/wire/android/util/DateAndTimeParsers.kt
Outdated
Show resolved
Hide resolved
|
@claude review |
Pull Request Review - Time Format Following Device SettingsSummaryThis PR fixes an issue where the Cells feature was not respecting the device's 12h/24h time format settings. The changes introduce new methods to use java.text.DateFormat for time formatting, which automatically adapts to the system's locale and time format preferences. Code Quality & Best PracticesStrengths
Issues & Concerns1. Inconsistent Removal of cellFileDateTimeFormat (core/ui-common/src/main/kotlin/com/wire/android/util/DateAndTimeParsers.kt:105-106) The old cellFileDateTimeFormat formatter was removed, but there is an inconsistency in the codebase. Impact: Medium - The code works but cleanup could be more complete. Recommendation: Verify all references are consistent. 2. Hardcoded Date Pattern (core/ui-common/src/main/kotlin/com/wire/android/util/DateAndTimeParsers.kt:171-172) While the time format now respects device settings, the date format is still hardcoded using SimpleDateFormat with pattern MMM dd, yyyy. Impact: High - This does not fully respect locale preferences
Recommendation: Consider using DateFormat.getDateInstance() for dates as well to fully respect locale preferences. Performance ConsiderationsNo major concerns
Security ConcernsNo security issues identified
Test CoverageMissing Tests The existing test file (DateAndTimeParsersTest.kt) does not include tests for the new functions:
Recommendation: Add tests to verify:
Summary of RecommendationsMust Fix
Should Consider
Nice to Have
VerdictOverall Assessment: Approve with Recommendations The core fix is solid and addresses the issue correctly. The use of DateFormat.getTimeInstance() is the right approach for respecting device time format settings. However, the PR would benefit from:
The code is production-ready but would be more robust with the recommended improvements. Review completed by Claude Code |
…evice-settings' into time-format-doesn't-follow-the-device-settings
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #4502 +/- ##
===========================================
- Coverage 48.73% 48.72% -0.01%
===========================================
Files 563 563
Lines 19493 19516 +23
Branches 3252 3253 +1
===========================================
+ Hits 9499 9510 +11
- Misses 8997 9008 +11
- Partials 997 998 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|



PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
For cells files, time should follow device format. If system is using 12h or 24h format, the app should follow this.
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.