Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoved the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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/com/philkes/notallyx/presentation/UiExtensions.kt (1)
601-616:⚠️ Potential issue | 🟠 MajorReplace
java.text.DateFormat.getDateTimeInstance(SHORT, SHORT)with Android's time format API to honor system 24-hour setting
java.text.DateFormat.getDateTimeInstance(SHORT, SHORT)formats based on locale but does not respect Android's system "Use 24-hour format" setting (Settings → Date & time). This affects all devices: a user inen_USlocale with 24-hour format enabled will still see 12-hour time (h:mm a) instead of 24-hour time (HH:mm).The Android-idiomatic fix is to use
android.text.format.DateFormat.getTimeFormat(context)for the time portion, which respects the system setting:DateFormat.TIMESTAMP_SHORT -> android.text.format.DateFormat.getTimeFormat(context).format(this) + " " + java.text.DateFormat.getDateInstance(java.text.DateFormat.SHORT).format(this)Alternatively, use
android.text.format.DateFormat.getBestDateTimePattern(locale, skeleton)where skeleton is"yMdHm"(24-hour) or"yMdhm"(12-hour), determined byandroid.text.format.DateFormat.is24HourFormat(context).Note: This requires passing
Contextto theDate.format()extension; refactor its signature if needed. The codebase already demonstrates awareness of this pattern inTimePickerFragment.kt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt` around lines 601 - 616, The TIMESTAMP_SHORT branch in the Date.format extension uses java.text.DateFormat.getDateTimeInstance(SHORT, SHORT) which ignores Android’s system 24-hour setting; change the Date.format signature to accept a Context and update DateFormat.TIMESTAMP_SHORT to build the string using android.text.format.DateFormat.getTimeFormat(context).format(this) for the time portion combined with java.text.DateFormat.getDateInstance(java.text.DateFormat.SHORT).format(this) for the date portion (or alternatively use android.text.format.DateFormat.getBestDateTimePattern(locale, skeleton) with android.text.format.DateFormat.is24HourFormat(context)); update all callers of Date.format(...) accordingly and use TimePickerFragment.kt as a reference for the pattern.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.kt (1)
15-15:utilslayer importing frompresentationlayer — optional refactor
format()is defined incom.philkes.notallyx.presentationand is now imported intoAlarmExtensions(andAutoRemoveDeletedNotesWorker) for debug log strings only. This introduces autils → presentationdependency that inverts the typical layer direction.Consider extracting a thin, context-free formatting helper (e.g., an
internalextension in autilssub-package, or inModelExtensions) that both the presentation and utils layers can share, keeping thepresentationpackage as a leaf consumer rather than a dependency ofutils.Since these calls are debug log-only, the practical runtime impact is zero, but the layer coupling is worth addressing in a follow-up.
Also applies to: 27-27, 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.kt` at line 15, The utils layer is importing presentation.format(), creating an inverted dependency; move the formatting logic into a shared, context-free extension that both layers can use (e.g., create an internal extension function named format() in a utils sub-package or ModelExtensions) and replace imports in AlarmExtensions and AutoRemoveDeletedNotesWorker to use that new shared helper; ensure the new helper has the same signature/behavior so calls in AlarmExtensions.format() references and AutoRemoveDeletedNotesWorker continue to work without importing com.philkes.notallyx.presentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/utils/AutoRemoveDeletedNotesWorker.kt`:
- Around line 42-43: In AutoRemoveDeletedNotesWorker (the log call building the
message with "Removing notes that have been deleted for $days days or more
(since: ${Date(before).format()"), close the unbalanced parenthesis in the log
string by appending a ')' after Date(before).format(), e.g. change the message
to "Removing notes that have been deleted for $days days or more (since:
${Date(before).format()})" so the logged output is well-formed.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt`:
- Around line 601-616: The TIMESTAMP_SHORT branch in the Date.format extension
uses java.text.DateFormat.getDateTimeInstance(SHORT, SHORT) which ignores
Android’s system 24-hour setting; change the Date.format signature to accept a
Context and update DateFormat.TIMESTAMP_SHORT to build the string using
android.text.format.DateFormat.getTimeFormat(context).format(this) for the time
portion combined with
java.text.DateFormat.getDateInstance(java.text.DateFormat.SHORT).format(this)
for the date portion (or alternatively use
android.text.format.DateFormat.getBestDateTimePattern(locale, skeleton) with
android.text.format.DateFormat.is24HourFormat(context)); update all callers of
Date.format(...) accordingly and use TimePickerFragment.kt as a reference for
the pattern.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/utils/AutoRemoveDeletedNotesWorker.kt`:
- Line 12: AutoRemoveDeletedNotesWorker.kt currently imports
com.philkes.notallyx.presentation.format which creates a utils → presentation
layer dependency; remove that import and either (a) move the format function
into a utils-level package (e.g., utils.formatter) and call that from
AutoRemoveDeletedNotesWorker, or (b) add a small formatting helper or use
standard DateTimeFormatter inside AutoRemoveDeletedNotesWorker so it depends
only on utils. Update references to format in AutoRemoveDeletedNotesWorker and
mirror the same refactor applied to AlarmExtensions.kt to keep layering
consistent.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.kt`:
- Line 15: The utils layer is importing presentation.format(), creating an
inverted dependency; move the formatting logic into a shared, context-free
extension that both layers can use (e.g., create an internal extension function
named format() in a utils sub-package or ModelExtensions) and replace imports in
AlarmExtensions and AutoRemoveDeletedNotesWorker to use that new shared helper;
ensure the new helper has the same signature/behavior so calls in
AlarmExtensions.format() references and AutoRemoveDeletedNotesWorker continue to
work without importing com.philkes.notallyx.presentation.
…NotesWorker.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Fixes #858
Summary by CodeRabbit