Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSplit date and time into separate overview and note-view preferences, introduced a TimeFormat enum and redesigned DateFormat with formatters, updated preferences, UI dialogs, adapters, view holders, and formatting helpers, removed the old apply-in-note-view toggle, and adjusted translations and documentation counts. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Settings as SettingsFragment
participant Dialog as DialogDatetimeFormatBinding
participant Prefs as NotallyXPreferences
participant Views as Activities/Adapters
User->>Settings: open settings
Settings->>Dialog: setupDateTimeFormat(overview/note)
Dialog->>User: show date & time dropdowns
User->>Dialog: select dateFormat, timeFormat
User->>Dialog: confirm
Dialog->>Prefs: save dateFormatX, timeFormatX
Prefs->>Views: emit updated preferences
Views->>Views: re-render timestamps using Date.format(dateFormat, timeFormat)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (1)
633-651: Consider using destructured values instead of re-reading preferences.The observer merges three LiveData sources but only uses the first destructured value (
time). The date/time formats are then re-read frommodel.preferencesinstead of using the values from the merge. While functionally correct, this pattern is inconsistent—either use the merged values or simplify the observer to only observelastExecutionPreference.♻️ Option 1: Use destructured values
lastExecutionPreference .merge(model.preferences.dateFormatOverview, model.preferences.timeFormatOverview) - .observe(viewLifecycleOwner) { (time, _, _) -> + .observe(viewLifecycleOwner) { (time, dateFormat, timeFormat) -> binding.PeriodicBackupLastExecution.apply { if (time != -1L) { isVisible = true text = Date(time) .format( - model.preferences.dateFormatOverview.value, - model.preferences.timeFormatOverview.value, + dateFormat, + timeFormat, ensureFullFormat = true, )🤖 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/activity/main/fragment/settings/SettingsFragment.kt` around lines 633 - 651, The observer on lastExecutionPreference.merge(...) currently destructures only the first value (time) and then re-reads model.preferences.dateFormatOverview/value and timeFormatOverview/value; change the lambda to destructure all three values (e.g., (time, dateFormatOverview, timeFormatOverview)) and use those destructured values when formatting the Date inside binding.PeriodicBackupLastExecution so you don't re-read model.preferences; update the observer in SettingsFragment.kt where lastExecutionPreference.merge(...) is observed to use the extracted date/time format values.
🤖 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/presentation/activity/note/reminders/RemindersActivity.kt`:
- Around line 470-471: The delete dialog currently forces a hardcoded full
format via reminder.dateTime.format(ensureFullFormat = true), causing a mismatch
with how reminders are rendered elsewhere (e.g. ReminderVH); change the dialog
to use the same user-configured note-view formatting as ReminderVH by removing
the forced ensureFullFormat flag and delegating to the shared/standard date-time
formatter used by ReminderVH (or call the same format invocation that ReminderVH
uses) so the dialog shows the reminder timestamp in the user’s configured
note-view format.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt`:
- Around line 366-396: The file-level SimpleDateFormat instances
(ISO_DATE_FORMAT, MM_DD_YY_FORMAT, DD_MM_YY_FORMAT, DD_MM_YY_FORMAT_GER,
TWENTY_FOUR_H_FORMAT, AM_PM_FORMAT) are not thread-safe and are used by the
DateFormat and TimeFormat enum format lambdas (and by the Date.format()
extension in UiExtensions.kt), so replace them with thread-safe usage: either
instantiate a new SimpleDateFormat inside each lambda/format call (create new
instances in the DateFormat and TimeFormat enum constructors or inside their
format lambdas) or, if targeting API 24+, switch to
java.time.format.DateTimeFormatter and java.time classes for immutable,
thread-safe formatting; update DateFormat.getText and any uses in Date.format()
to call the new per-call/thread-safe formatter APIs (reference DateFormat,
TimeFormat, ISO_DATE_FORMAT, MM_DD_YY_FORMAT, DD_MM_YY_FORMAT,
DD_MM_YY_FORMAT_GER, TWENTY_FOUR_H_FORMAT, AM_PM_FORMAT and the Date.format()
extension).
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Around line 633-651: The observer on lastExecutionPreference.merge(...)
currently destructures only the first value (time) and then re-reads
model.preferences.dateFormatOverview/value and timeFormatOverview/value; change
the lambda to destructure all three values (e.g., (time, dateFormatOverview,
timeFormatOverview)) and use those destructured values when formatting the Date
inside binding.PeriodicBackupLastExecution so you don't re-read
model.preferences; update the observer in SettingsFragment.kt where
lastExecutionPreference.merge(...) is observed to use the extracted date/time
format values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa2d7f10-b04e-417c-a55c-8175391bcd1e
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (29)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/NotallyFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/PickNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/reminder/ReminderAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/reminder/ReminderVH.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/NotallyXPreferences.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.ktapp/src/main/res/layout/dialog_datetime_format.xmlapp/src/main/res/layout/fragment_settings.xmlapp/src/main/res/values-cs/strings.xmlapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-fr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-pl/strings.xmlapp/src/main/res/values-pt-rBR/strings.xmlapp/src/main/res/values-ro/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-uk/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values-zh-rTW/strings.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (12)
- app/src/main/res/values-fr/strings.xml
- app/src/main/res/values-es/strings.xml
- app/src/main/res/values-it/strings.xml
- app/src/main/res/values-ro/strings.xml
- app/src/main/res/values-uk/strings.xml
- app/src/main/res/values-zh-rCN/strings.xml
- app/src/main/res/values-cs/strings.xml
- app/src/main/res/values-de/strings.xml
- app/src/main/res/values-zh-rTW/strings.xml
- app/src/main/res/values-ru/strings.xml
- app/src/main/res/values-pt-rBR/strings.xml
- app/src/main/res/values-pl/strings.xml
Comment on lines
+470
to
+471
| .setMessage( | ||
| "${reminder.dateTime.format()}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}" | ||
| "${reminder.dateTime.format(ensureFullFormat = true)}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Date.format(...) definition(s) =="
rg -n -C4 --type=kt '\bfun\s+Date\.format\s*\(' app/src/main/java
echo
echo "== Reminder timestamp call sites =="
rg -n -C2 --type=kt 'dateTime\.format\(' \
app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.kt \
app/src/main/java/com/philkes/notallyx/presentation/view/main/reminder/ReminderVH.ktRepository: Crustack/NotallyX
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding Date.format(...) definition(s) =="
find app/src/main/java -name "*.kt" -exec rg -l "fun.*Date\.format" {} \;
echo
rg -n -C4 "fun.*Date\.format" app/src/main/java
echo
echo "== Reminder timestamp call sites in RemindersActivity.kt =="
rg -n -C2 "dateTime\.format" app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.kt
echo
echo "== Reminder timestamp call sites in ReminderVH.kt =="
rg -n -C2 "dateTime\.format" app/src/main/java/com/philkes/notallyx/presentation/view/main/reminder/ReminderVH.ktRepository: Crustack/NotallyX
Length of output: 1816
Align the delete dialog with the configured note-view formats.
The delete confirmation dialog uses format(ensureFullFormat = true), which applies hardcoded defaults (DateFormat.DD_MM_YY_GER and TimeFormat.TWENTY_FOUR_H). Meanwhile, ReminderVH renders the same reminder timestamp with the user's configured note-view preferences. This creates a UX inconsistency where the same reminder appears in different formats across your app.
💡 Proposed change
- "${reminder.dateTime.format(ensureFullFormat = true)}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}"
+ "${reminder.dateTime.format(
+ preferences.dateFormatNoteView.value,
+ preferences.timeFormatNoteView.value,
+ ensureFullFormat = true,
+ )}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}"📝 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.
Suggested change
| .setMessage( | |
| "${reminder.dateTime.format()}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}" | |
| "${reminder.dateTime.format(ensureFullFormat = true)}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}" | |
| .setMessage( | |
| "${reminder.dateTime.format( | |
| preferences.dateFormatNoteView.value, | |
| preferences.timeFormatNoteView.value, | |
| ensureFullFormat = true, | |
| )}\n${reminder.repetition?.toText(this) ?: getString(R.string.reminder_no_repetition)}" |
🤖 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/activity/note/reminders/RemindersActivity.kt`
around lines 470 - 471, The delete dialog currently forces a hardcoded full
format via reminder.dateTime.format(ensureFullFormat = true), causing a mismatch
with how reminders are rendered elsewhere (e.g. ReminderVH); change the dialog
to use the same user-configured note-view formatting as ReminderVH by removing
the forced ensureFullFormat flag and delegating to the shared/standard date-time
formatter used by ReminderVH (or call the same format invocation that ReminderVH
uses) so the dialog shows the reminder timestamp in the user’s configured
note-view format.
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt (1)
366-394: Limit formatter helper visibility to avoid API surface creep.
ThreadLocalDateFormatandThreadLocalDateInstanceappear internal to this file; making themprivateprevents accidental external coupling.Proposed diff
-class ThreadLocalDateFormat(private val format: String, private val locale: Locale) { +private class ThreadLocalDateFormat(private val format: String, private val locale: Locale) { @@ -class ThreadLocalDateInstance(private val style: Int) { +private class ThreadLocalDateInstance(private val style: Int) {🤖 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/viewmodel/preference/Preference.kt` around lines 366 - 394, The two helper classes ThreadLocalDateFormat and ThreadLocalDateInstance are package-visible but only used internally here; change their declarations to private (make the classes private) so they are not exposed from this file, e.g., update the class headers for ThreadLocalDateFormat and ThreadLocalDateInstance to be private to limit the API surface and prevent external coupling while keeping their constructors and methods as-is.
🤖 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/presentation/viewmodel/preference/Preference.kt`:
- Around line 412-417: Replace the fixed millisecond subtraction in the enum
getText(context: Context): String implementations (the branches that handle NONE
vs "yesterday") with day-based arithmetic: create a small helper (e.g.,
getYesterdayCalendar() or getYesterdayDate()) that uses Calendar.getInstance(),
calls add(Calendar.DAY_OF_YEAR, -1) and returns the Date/Calendar/Instant
needed, and call that helper from both enum getText implementations instead of
Date(System.currentTimeMillis() - 86400000); ensure the helper is reused by the
other enum branch referenced in the review so DST transitions produce correct
"yesterday" values.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt`:
- Around line 366-394: The two helper classes ThreadLocalDateFormat and
ThreadLocalDateInstance are package-visible but only used internally here;
change their declarations to private (make the classes private) so they are not
exposed from this file, e.g., update the class headers for ThreadLocalDateFormat
and ThreadLocalDateInstance to be private to limit the API surface and prevent
external coupling while keeping their constructors and methods as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86c74e9d-fdf5-4260-a4b1-a97dc373036e
📒 Files selected for processing (3)
.github/workflows/update-translation-coverage.yamlapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/update-translation-coverage.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.kt
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.kt
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #842
Closes #930
notallyx_issues_930_2.webm
Summary by CodeRabbit
New Features
Changes