Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds "pin to status bar": database schema v10 with new BaseNote.isPinnedToStatus and migration; DAO, model, ViewModel, importer, and UI changes to support pinning; new PinnedNotificationManager and reminder/notification refactors; permission flows and action-mode extraction; resource and manifest adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Activity as EditActivity/MainActivity
participant Handler as NoteActionHandler
participant Perm as PermissionManager
participant DAO as BaseNoteDao
participant PinMgr as PinnedNotificationManager
participant Notif as NotificationManager
User->>Activity: Tap "Pin to status bar"
Activity->>Handler: pinToStatus()
Handler->>Perm: request notification permission
Perm-->>Handler: granted / denied
alt granted
Handler->>DAO: updatePinnedToStatus(id, true)
Handler->>PinMgr: notify(context, note)
PinMgr->>Notif: notify(NOTIFICATION_TAG, note.id)
else denied
Handler->>Activity: handleRejection(msgResId)
end
alt unpin
Handler->>DAO: updatePinnedToStatus(id, false)
Handler->>PinMgr: cancel(context, note.id)
PinMgr->>Notif: cancel(NOTIFICATION_TAG, note.id)
end
sequenceDiagram
participant System
participant Receiver as ReminderReceiver
participant DAO as BaseNoteDao
participant Alarm as AlarmManager
participant PinMgr as PinnedNotificationManager
participant Notif as NotificationManager
System->>Receiver: ACTION_DELETE_NOTE / ACTION_UNPIN_NOTE / ACTION_UPDATE_NOTIFICATIONS
Receiver->>DAO: fetch/update or move note
alt delete
Receiver->>Alarm: cancel alarms for note.reminders
Receiver->>PinMgr: cancel(context, note.id)
else unpin
Receiver->>DAO: updatePinnedToStatus(note.id, false)
Receiver->>PinMgr: cancel(context, note.id)
else update
Receiver->>DAO: get note
Receiver->>Notif: repost reminder notifications for note (createNotification)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
![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: 5
🧹 Nitpick comments (8)
app/src/main/java/com/philkes/notallyx/utils/PinnedNotificationManager.kt (1)
62-73: Consider edge case: Summary notification cleanup on pre-M devices.The summary notification is only canceled on Android M+ where
activeNotificationsis available. On older devices, stale summary notifications may persist after all pinned notes are unpinned.💡 Potential improvement for pre-M devices
Since Android M (API 23) is the minimum practical target for most apps today, this is likely acceptable. However, if supporting older devices, consider tracking pinned note count separately:
fun cancel(context: Context, noteId: Long) { val manager = context.getSystemService<NotificationManager>()!! manager.cancel(NOTIFICATION_TAG, noteId.toInt()) if ( Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && manager.activeNotifications.none { it.tag == NOTIFICATION_TAG && it.id != noteId.toInt() } ) { manager.cancel(SUMMARY_ID) + } else if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) { + // Pre-M: Always cancel summary; it will be recreated if needed + manager.cancel(SUMMARY_ID) } }🤖 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/PinnedNotificationManager.kt` around lines 62 - 73, The cancel(...) method only removes SUMMARY_ID when Build.VERSION.SDK_INT >= M using NotificationManager.activeNotifications, leaving stale summary notifications on pre-M devices; update the logic to maintain a persisted pinned-note counter (e.g., in SharedPreferences) that is incremented when creating/posting a pinned notification and decremented in cancel(noteId), and on decrement to zero call manager.cancel(SUMMARY_ID). Modify the functions that post pinned notifications (where NOTIFICATION_TAG and SUMMARY_ID are used) to update this counter so cancel(...) can rely on it for pre-M devices when activeNotifications is unavailable.app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt (2)
302-311: Redundant filter:getAllPinnedToStatusNotes()already filters byisPinnedToStatus = 1.The
.filter { it.isPinnedToStatus }on line 306 is redundant since the query on line 304 already returns only notes whereisPinnedToStatus = 1.♻️ Remove redundant filter
private suspend fun restorePinnedNotifications(context: Context) { val baseNoteDao = getDatabase(context).getBaseNoteDao() val allNotes = baseNoteDao.getAllPinnedToStatusNotes() allNotes - .filter { it.isPinnedToStatus } .forEach { note -> Log.d(TAG, "restorePinnedNotifications: Pinning noteID: ${note.id} to status bar") PinnedNotificationManager.notify(context, note) } }🤖 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/ReminderReceiver.kt` around lines 302 - 311, The restorePinnedNotifications function applies an unnecessary runtime filter after fetching pinned notes; remove the redundant .filter { it.isPinnedToStatus } call in restorePinnedNotifications since getAllPinnedToStatusNotes() already returns only notes with isPinnedToStatus = 1, and then iterate directly over allNotes and call PinnedNotificationManager.notify(context, note) as before.
153-156: Remove commented-out code.Dead code comments should be removed. If this logic is needed for future reference, it should be tracked in a separate issue or commit history.
♻️ Remove commented code
manager.activeNotifications.ofNote(noteId).forEach { notification -> val reminderId = notification.id Log.d(TAG, "Updating notification for noteId: $noteId reminderId: $reminderId") notify( context, noteId, reminderId.toLong(), schedule = false, isOnlyUpdate = true, ) } - // val mostRecentReminder = note.reminders.findLastNotified() ?: return - // if (mostRecentReminder.isNotificationVisible) { - // notify(context, note.id, mostRecentReminder.id, schedule = false) - // } } }🤖 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/ReminderReceiver.kt` around lines 153 - 156, Remove the dead commented-out reminder logic in ReminderReceiver: delete the three commented lines that reference mostRecentReminder, note.reminders.findLastNotified(), and the notify(...) call so the class/function is not cluttered with commented code; if the behavior must be preserved for future reference, open an issue or rely on VCS history rather than leaving commented code in ReminderReceiver.app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt (1)
336-338: Consider batching label updates for better performance.Currently, each note's labels are updated individually in a loop. For a large selection, this could result in many database operations.
♻️ Suggested batch update approach
// Consider collecting updates and using a batch DAO method: val updates = baseNotes.zip(updatedBaseNotesLabels) .map { (baseNote, labels) -> baseNote.id to labels } baseModel.updateBaseNotesLabels(updates) // batch operation🤖 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/ModelFolderObserver.kt` around lines 336 - 338, The loop calls baseModel.updateBaseNoteLabels for each note causing many DB ops; instead collect pairs of note id and labels from baseNotes.zip(updatedBaseNotesLabels) and call a new batch method (e.g., baseModel.updateBaseNotesLabels(updates: List<Pair<Id, Labels>>) to perform one batched update; add the batch method to the DAO/repository and use it from ModelFolderObserver (replace the forEach block referencing baseModel.updateBaseNoteLabels) to reduce round-trips and improve performance.app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt (1)
163-166: Minor SQL style inconsistency: use=instead of==.The query uses
isPinnedToStatus == 1whilegetAllPinnedToStatusNotesusesisPinnedToStatus = 1. Both work in SQLite, but using=is the standard SQL equality operator and maintains consistency.♻️ Suggested fix
`@Query`( - "SELECT * FROM BaseNote WHERE folder = 'NOTES' AND ((reminders IS NOT NULL AND reminders != '[]') OR isPinnedToStatus == 1)" + "SELECT * FROM BaseNote WHERE folder = 'NOTES' AND ((reminders IS NOT NULL AND reminders != '[]') OR isPinnedToStatus = 1)" ) suspend fun getAllWithRemindersOrPinned(): List<BaseNote>🤖 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/data/dao/BaseNoteDao.kt` around lines 163 - 166, The SQL query in getAllWithRemindersOrPinned uses nonstandard equality operator `==`; update the WHERE clause to use `=` for consistency (change `isPinnedToStatus == 1` to `isPinnedToStatus = 1`) so it matches the style used in getAllPinnedToStatusNotes and standard SQL conventions.app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt (1)
517-535: Redundant permission check inrefreshStatusBarPin.When
note.isPinnedToStatusistrue, the permission was already granted during thepinToStatus()call that set this flag. The additionalcheckNotificationPermissioncall on lines 526-531 is redundant and adds unnecessary complexity.♻️ Simplified implementation
fun Activity.refreshStatusBarPin(note: BaseNote) { - val refresh = { - if (note.isPinnedToStatus) { - PinnedNotificationManager.notify(this, note) - } else { - PinnedNotificationManager.cancel(this, note.id) - } - } if (note.isPinnedToStatus) { - checkNotificationPermission( - REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS, - alsoCheckAlarmPermission = false, - ) { - refresh() - } + PinnedNotificationManager.notify(this, note) } else { - refresh() + PinnedNotificationManager.cancel(this, note.id) } }🤖 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/NoteActionHandler.kt` around lines 517 - 535, The refreshStatusBarPin function performs an unnecessary checkNotificationPermission when note.isPinnedToStatus is true even though permission was already handled in pinToStatus; remove the conditional permission branch and always invoke the refresh lambda directly for both pinned and unpinned cases (i.e., eliminate the checkNotificationPermission call inside refreshStatusBarPin), leaving only the refresh logic that calls PinnedNotificationManager.notify(this, note) or PinnedNotificationManager.cancel(this, note.id).app/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.kt (1)
51-58:cancelPinAndRemindersunconditionally cancels pinned notification regardless ofisPinnedToStatusstate.The function calls
PinnedNotificationManager.cancel(this, noteId)without checking if the note is actually pinned. While this is a no-op for non-existent notifications, it's semantically inconsistent and could mask bugs. Consider adding a conditional check or documenting this as intentional "best-effort cleanup" behavior.♻️ Suggested conditional cancellation
fun Context.cancelPinAndReminders(noteId: Long, reminders: List<Reminder>) { reminders.forEach { reminder -> cancelReminder(noteId, reminder.id) } + // Always attempt to cancel pinned notification for cleanup purposes PinnedNotificationManager.cancel(this, noteId) } + +// Alternative: Add overload that respects isPinnedToStatus +fun Context.cancelPinAndReminders(note: BaseNote) { + note.reminders.forEach { reminder -> cancelReminder(note.id, reminder.id) } + if (note.isPinnedToStatus) { + PinnedNotificationManager.cancel(this, note.id) + } +}🤖 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` around lines 51 - 58, The cancelPinAndReminders functions unconditionally call PinnedNotificationManager.cancel(this, noteId); update them to only cancel pinned notifications by checking the note's isPinnedToStatus before calling PinnedNotificationManager.cancel (e.g., in fun Context.cancelPinAndReminders(noteId: Long, reminders: List<Reminder>) check the associated note or add an isPinned flag parameter, and in fun Context.cancelPinAndReminders(notes: List<BaseNote>) use note.isPinnedToStatus to decide per-note cancellation), ensuring reminders are still cancelled unconditionally; alternatively, if intentional, add a short comment documenting the best-effort behavior.app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.kt (1)
26-38: Duplicate code:contentTextandbigTextare computed identically.Lines 26-31 and 33-38 contain the exact same logic. Extract to a single variable.
♻️ Remove duplication
- val contentText = + val bodyText = if (note.type == com.philkes.notallyx.data.model.Type.LIST) { note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } } else { note.body } - val bigText = - if (note.type == com.philkes.notallyx.data.model.Type.LIST) { - note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } - } else { - note.body - } val notification = NotificationCompat.Builder(this, channelId) .apply { setSubText(subtext) setSmallIcon(iconResId) setCategory(NotificationCompat.CATEGORY_REMINDER) setContentTitle(note.title.ifEmpty { getString(R.string.note) }) - setContentText(contentText) - setStyle(NotificationCompat.BigTextStyle().bigText(bigText)) + setContentText(bodyText) + setStyle(NotificationCompat.BigTextStyle().bigText(bodyText))🤖 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/RemindersUtils.kt` around lines 26 - 38, The code computes identical values for contentText and bigText; extract the shared logic into a single local val (e.g., formattedText or formattedBody) that checks note.type == com.philkes.notallyx.data.model.Type.LIST and builds the joinToString for note.items or uses note.body, then assign contentText = formattedText and bigText = formattedText (replace the duplicated blocks in RemindersUtils.kt). Ensure you reference the same note variable and preserve the "✅"/"🔳" and newline behavior.
🤖 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/main/ModelFolderObserver.kt`:
- Around line 229-247: The loading flag is cleared in moveNotes' finally block
before the async DB work started by baseModel.moveBaseNotes(...) completes;
change the flow so baseModel.actionMode.loading.value is reset only when the
background operation actually finishes: either make
baseModel.moveBaseNotes(folderTo) a suspend function and call it from a
coroutine (e.g., lifecycleScope.launch { try { ... await moveBaseNotes(...) }
finally { loading.value = false } }), or have moveBaseNotes return a
Job/Deferred and call .invokeOnCompletion / await that Job before resetting
baseModel.actionMode.loading.value; reference moveNotes,
baseModel.moveBaseNotes(folderTo), baseModel.moveBaseNotes(ids, folderFrom) and
baseModel.actionMode.loading.value when implementing the fix.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt`:
- Line 64: Duplicate pinned notification restoration occurs because both
NotallyXApplication.onCreate() and ReminderReceiver (BOOT_COMPLETED handler)
call restorePinnedNotifications(context); remove the redundant call from
ReminderReceiver (the BOOT_COMPLETED branch) so only
NotallyXApplication.onCreate() invokes restorePinnedNotifications(context),
ensuring pinned notifications are restored once on boot.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt`:
- Around line 301-308: The broadcast to ReminderReceiver via Intent(app,
ReminderReceiver::class.java) is safe as an explicit intent, but the manifest
currently exposes the receiver; update the ReminderReceiver AndroidManifest
entry to set android:exported="false" (instead of "true") so external apps
cannot receive or invoke ACTION_UPDATE_NOTIFICATIONS/EXTRA_NOTE_ID broadcasts;
locate the ReminderReceiver declaration in the manifest and change the exported
attribute accordingly.
In `@app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt`:
- Around line 448-465: Update getOpenNoteIntent to include
Intent.FLAG_ACTIVITY_NEW_TASK in the addFlags call so intents launched from
non-Activity contexts (e.g., notifications) will start correctly; modify the
addFlags invocation in getOpenNoteIntent (used by getOpenNotePendingIntent and
callers like PinnedNotificationManager.notify and
RemindersUtils.createNotification) to add FLAG_ACTIVITY_NEW_TASK alongside
FLAG_ACTIVITY_SINGLE_TOP and FLAG_ACTIVITY_CLEAR_TOP.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 221-223: The ZIP import drops the status-bar pin because the
backup DAO query (used by getAllWithRemindersOrPinned) does not select the
isPinnedToStatus column and toBaseNote constructs imported BaseNote with the
default false; update the import/backup query in BaseNoteDao (the query backing
getAllWithRemindersOrPinned) to include the is_pinned_to_status/isPinnedToStatus
column, and update toBaseNote (or the import mapping code) to map that column
into the BaseNote.isPinnedToStatus field so that cancelPinAndReminders(...) and
pinAndScheduleReminders(...) operate on the correct pinned state after import.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt`:
- Around line 163-166: The SQL query in getAllWithRemindersOrPinned uses
nonstandard equality operator `==`; update the WHERE clause to use `=` for
consistency (change `isPinnedToStatus == 1` to `isPinnedToStatus = 1`) so it
matches the style used in getAllPinnedToStatusNotes and standard SQL
conventions.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt`:
- Around line 336-338: The loop calls baseModel.updateBaseNoteLabels for each
note causing many DB ops; instead collect pairs of note id and labels from
baseNotes.zip(updatedBaseNotesLabels) and call a new batch method (e.g.,
baseModel.updateBaseNotesLabels(updates: List<Pair<Id, Labels>>) to perform one
batched update; add the batch method to the DAO/repository and use it from
ModelFolderObserver (replace the forEach block referencing
baseModel.updateBaseNoteLabels) to reduce round-trips and improve performance.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt`:
- Around line 517-535: The refreshStatusBarPin function performs an unnecessary
checkNotificationPermission when note.isPinnedToStatus is true even though
permission was already handled in pinToStatus; remove the conditional permission
branch and always invoke the refresh lambda directly for both pinned and
unpinned cases (i.e., eliminate the checkNotificationPermission call inside
refreshStatusBarPin), leaving only the refresh logic that calls
PinnedNotificationManager.notify(this, note) or
PinnedNotificationManager.cancel(this, note.id).
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt`:
- Around line 302-311: The restorePinnedNotifications function applies an
unnecessary runtime filter after fetching pinned notes; remove the redundant
.filter { it.isPinnedToStatus } call in restorePinnedNotifications since
getAllPinnedToStatusNotes() already returns only notes with isPinnedToStatus =
1, and then iterate directly over allNotes and call
PinnedNotificationManager.notify(context, note) as before.
- Around line 153-156: Remove the dead commented-out reminder logic in
ReminderReceiver: delete the three commented lines that reference
mostRecentReminder, note.reminders.findLastNotified(), and the notify(...) call
so the class/function is not cluttered with commented code; if the behavior must
be preserved for future reference, open an issue or rely on VCS history rather
than leaving commented code in ReminderReceiver.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.kt`:
- Around line 26-38: The code computes identical values for contentText and
bigText; extract the shared logic into a single local val (e.g., formattedText
or formattedBody) that checks note.type ==
com.philkes.notallyx.data.model.Type.LIST and builds the joinToString for
note.items or uses note.body, then assign contentText = formattedText and
bigText = formattedText (replace the duplicated blocks in RemindersUtils.kt).
Ensure you reference the same note variable and preserve the "✅"/"🔳" and
newline behavior.
In `@app/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.kt`:
- Around line 51-58: The cancelPinAndReminders functions unconditionally call
PinnedNotificationManager.cancel(this, noteId); update them to only cancel
pinned notifications by checking the note's isPinnedToStatus before calling
PinnedNotificationManager.cancel (e.g., in fun
Context.cancelPinAndReminders(noteId: Long, reminders: List<Reminder>) check the
associated note or add an isPinned flag parameter, and in fun
Context.cancelPinAndReminders(notes: List<BaseNote>) use note.isPinnedToStatus
to decide per-note cancellation), ensuring reminders are still cancelled
unconditionally; alternatively, if intentional, add a short comment documenting
the best-effort behavior.
In `@app/src/main/java/com/philkes/notallyx/utils/PinnedNotificationManager.kt`:
- Around line 62-73: The cancel(...) method only removes SUMMARY_ID when
Build.VERSION.SDK_INT >= M using NotificationManager.activeNotifications,
leaving stale summary notifications on pre-M devices; update the logic to
maintain a persisted pinned-note counter (e.g., in SharedPreferences) that is
incremented when creating/posting a pinned notification and decremented in
cancel(noteId), and on decrement to zero call manager.cancel(SUMMARY_ID). Modify
the functions that post pinned notifications (where NOTIFICATION_TAG and
SUMMARY_ID are used) to update this counter so cancel(...) can rely on it for
pre-M devices when activeNotifications is unavailable.
🪄 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: cf409de0-9ecd-4d5d-ade1-e81c7e88481e
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (38)
app/schemas/com.philkes.notallyx.data.NotallyDatabase/10.jsonapp/src/main/java/com/philkes/notallyx/NotallyXApplication.ktapp/src/main/java/com/philkes/notallyx/data/NotallyDatabase.ktapp/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.ktapp/src/main/java/com/philkes/notallyx/data/dao/DaoExtensions.ktapp/src/main/java/com/philkes/notallyx/data/model/BaseNote.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.ktapp/src/main/java/com/philkes/notallyx/presentation/view/note/action/MoreNoteBottomSheet.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.ktapp/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/PinnedNotificationManager.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/res/drawable/notification_pin.xmlapp/src/main/res/drawable/notification_unpin.xmlapp/src/main/res/drawable/pinboard.xmlapp/src/main/res/drawable/pinboard_filled.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
app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt
Show resolved
Hide resolved
| rescheduleAlarms(context) | ||
| } | ||
| restoreRemindersNotifications(context) | ||
| restorePinnedNotifications(context) |
Contributor
There was a problem hiding this comment.
Duplicate pinned notification restoration on boot.
Both NotallyXApplication.onCreate() (see context snippet 1) and ReminderReceiver.BOOT_COMPLETED call restorePinnedNotifications(). Since Application.onCreate() runs before BOOT_COMPLETED broadcast delivery, pinned notifications will be posted twice on device boot.
🔧 Suggested fix: Remove from one location
Remove the call from ReminderReceiver since NotallyXApplication already handles it:
intent.action == Intent.ACTION_BOOT_COMPLETED -> {
if (canScheduleExactAlarms) {
rescheduleAlarms(context)
}
restoreRemindersNotifications(context)
- restorePinnedNotifications(context)
}Or alternatively, remove from NotallyXApplication.onCreate() and keep only the boot receiver handling.
Also applies to: 302-311
🤖 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/ReminderReceiver.kt`
at line 64, Duplicate pinned notification restoration occurs because both
NotallyXApplication.onCreate() and ReminderReceiver (BOOT_COMPLETED handler)
call restorePinnedNotifications(context); remove the redundant call from
ReminderReceiver (the BOOT_COMPLETED branch) so only
NotallyXApplication.onCreate() invokes restorePinnedNotifications(context),
ensuring pinned notifications are restored once on boot.
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.kt
Show resolved
Hide resolved
Contributor
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 (1)
app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt (1)
126-156:⚠️ Potential issue | 🟠 MajorSerialize
isPinnedToStatusintoJson()too.
toBaseNote()now reads the field, butBaseNote.toJson()above still never writes it. AnytoJson()round-trip will silently clear status-bar pins.Suggested fix
fun BaseNote.toJson(): String { val jsonObject = JSONObject() .put("type", type.name) .put("color", color) .put("title", title) .put("pinned", pinned) + .put("isPinnedToStatus", isPinnedToStatus) .put("timestamp", timestamp) .put("modifiedTimestamp", modifiedTimestamp) .put("labels", JSONArray(labels))🤖 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/data/model/ModelExtensions.kt` around lines 126 - 156, The toBaseNote() deserializer reads the isPinnedToStatus field (pinnedToStatusBar) but BaseNote.toJson() never writes it, so round-trips lose the status-bar pin; update BaseNote.toJson() (or the serializer used for BaseNote) to emit the boolean property using the exact key "isPinnedToStatus" from the pinnedToStatusBar field (or BaseNote.isPinnedToStatus) so serialization and deserialization are symmetric.
♻️ Duplicate comments (2)
app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
323-331:⚠️ Potential issue | 🔴 CriticalZIP import will now fail before the first note is mapped.
This code calls
getColumnIndexOrThrow("isPinnedToStatus"), but the cursor built inimportZip()still does not project that column. That turns the old “flag gets dropped” bug into an import-time crash. AddisPinnedToStatusto thecolumnsarray and fall back tofalsewhen the source backup predates the column.Run this read-only check to verify the missing projection:
#!/bin/bash set -euo pipefail echo "# BaseNote projection used during ZIP import" sed -n '124,146p' app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt echo echo "# isPinnedToStatus lookup in cursor mapper" sed -n '323,332p' app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.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/utils/backup/ImportExtensions.kt` around lines 323 - 331, The ZIP import crashes because ImportExtensions.importZip() does not include "isPinnedToStatus" in the columns projection but the cursor mapping code uses getColumnIndexOrThrow("isPinnedToStatus"); update the projection (the columns array used by importZip()) to include "isPinnedToStatus" and change the mapper to use getColumnIndex("isPinnedToStatus") with a fallback of false (keep the existing when/0/1 mapping) so older backups that predate the column will not crash.app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
657-660:⚠️ Potential issue | 🟠 MajorGive callers a real completion callback.
This launches and returns immediately, then invokes
callableonDispatchers.IOonly after a successful move.ModelFolderObserver.moveNotes()uses that callback to clearloading.value, so failures leave loading stuck and the success path pushes a LiveData/UI update off-main.Suggested fix
- fun moveBaseNotes(ids: LongArray, folder: Folder, callable: (() -> Unit)? = null) { - viewModelScope.launch(Dispatchers.IO) { - app.moveBaseNotes(baseNoteDao, ids, folder) - callable?.invoke() - } - } + fun moveBaseNotes(ids: LongArray, folder: Folder, callable: (() -> Unit)? = null) = + viewModelScope.launch { + try { + withContext(Dispatchers.IO) { + app.moveBaseNotes(baseNoteDao, ids, folder) + } + } finally { + callable?.invoke() + } + }Run this read-only check to confirm the callback dispatch and the type behind
loading:#!/bin/bash set -euo pipefail echo "# moveBaseNotes callback dispatch" sed -n '650,662p' app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt echo echo "# ActionMode definition" fd 'ActionMode.kt' app/src/main/java | xargs -r sed -n '1,220p' echo echo "# NotNullLiveData implementation" fd 'NotNullLiveData.kt' app/src/main/java | xargs -r sed -n '1,220p'🤖 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/BaseNoteModel.kt` around lines 657 - 660, The moveBaseNotes implementation currently launches on Dispatchers.IO and only invokes callable on the IO thread after a successful move; change it to always invoke callable on the main thread regardless of success or failure. Surround the app.moveBaseNotes(baseNoteDao, ids, folder) call with try/catch/finally inside viewModelScope.launch(Dispatchers.IO), and in the finally block switch to Dispatchers.Main (e.g., withContext(Dispatchers.Main)) to call callable?.invoke() so callers (like ModelFolderObserver.moveNotes) get a real completion callback on the UI thread even when the move fails; keep exception handling (log or rethrow) as appropriate.
🤖 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/main/ModelFolderObserver.kt`:
- Around line 258-269: The undo currently reinserts notes but their attachment
files were already deleted by
baseModel.deleteSelectedBaseNotes()/BaseNoteModel.deleteBaseNotes(...); change
the flow so files are not permanently removed until the Snackbar undo window
passes: either (1) modify BaseNoteModel.deleteBaseNotes to only mark rows
deleted and return the list of attachment file paths (or accept a deferDeletion
flag) and then perform physical file deletion in the Snackbar dismiss callback
when no undo occurred, or (2) capture the attachment file paths before calling
baseModel.deleteSelectedBaseNotes() and, if the user taps Undo (setAction ->
baseModel.saveNotes(removedNotes)), restore those files from a temporary staging
area (or skip deleting them). Update ModelFolderObserver (the positive button
handler and the Snackbar setAction/dismiss logic) to coordinate this deferred
deletion behavior and use the unique symbols
baseModel.deleteSelectedBaseNotes(), BaseNoteModel.deleteBaseNotes(...), and
baseModel.saveNotes(removedNotes) to locate and implement the changes.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 631-632: The loop that calls activity.refreshStatusBarPin for
every note returned by baseNoteDao.getByIds(ids) should only re-notify notes
that are currently in Folder.NOTES; change the logic around
baseNoteDao.getByIds(ids) / updatedNotes.forEach {
activity.refreshStatusBarPin(it) } to filter updatedNotes to only those whose
folder equals Folder.NOTES (or use the note property/enum that indicates folder
membership) before calling activity.refreshStatusBarPin, so archived or
non-NOTES pins aren’t surfaced briefly and then removed by the existing
DaoExtensions behavior.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt`:
- Around line 126-156: The toBaseNote() deserializer reads the isPinnedToStatus
field (pinnedToStatusBar) but BaseNote.toJson() never writes it, so round-trips
lose the status-bar pin; update BaseNote.toJson() (or the serializer used for
BaseNote) to emit the boolean property using the exact key "isPinnedToStatus"
from the pinnedToStatusBar field (or BaseNote.isPinnedToStatus) so serialization
and deserialization are symmetric.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 657-660: The moveBaseNotes implementation currently launches on
Dispatchers.IO and only invokes callable on the IO thread after a successful
move; change it to always invoke callable on the main thread regardless of
success or failure. Surround the app.moveBaseNotes(baseNoteDao, ids, folder)
call with try/catch/finally inside viewModelScope.launch(Dispatchers.IO), and in
the finally block switch to Dispatchers.Main (e.g.,
withContext(Dispatchers.Main)) to call callable?.invoke() so callers (like
ModelFolderObserver.moveNotes) get a real completion callback on the UI thread
even when the move fails; keep exception handling (log or rethrow) as
appropriate.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 323-331: The ZIP import crashes because
ImportExtensions.importZip() does not include "isPinnedToStatus" in the columns
projection but the cursor mapping code uses
getColumnIndexOrThrow("isPinnedToStatus"); update the projection (the columns
array used by importZip()) to include "isPinnedToStatus" and change the mapper
to use getColumnIndex("isPinnedToStatus") with a fallback of false (keep the
existing when/0/1 mapping) so older backups that predate the column will not
crash.
🪄 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: 9f9c55cb-3a28-4725-a18c-92d9eafc5860
📒 Files selected for processing (11)
app/src/main/AndroidManifest.xmlapp/src/main/java/com/philkes/notallyx/data/imports/evernote/EvernoteImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/txt/PlainTextImporter.ktapp/src/main/java/com/philkes/notallyx/data/model/BaseNote.ktapp/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/XmlParserExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/philkes/notallyx/data/model/BaseNote.kt
- app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt
Comment on lines
+258
to
+269
| .setPositiveButton(R.string.delete) { _, _ -> | ||
| val removedNotes = baseModel.actionMode.selectedNotes.values.toList() | ||
| baseModel.deleteSelectedBaseNotes() | ||
| Snackbar.make( | ||
| activity.findViewById(R.id.DrawerLayout), | ||
| activity.getQuantityString( | ||
| R.plurals.deleted_selected_notes, | ||
| removedNotes.size, | ||
| ), | ||
| Snackbar.LENGTH_SHORT, | ||
| ) | ||
| .apply { setAction(R.string.undo) { baseModel.saveNotes(removedNotes) } } |
Contributor
There was a problem hiding this comment.
Undo here restores rows, not the deleted files.
baseModel.deleteSelectedBaseNotes() goes through BaseNoteModel.deleteBaseNotes(...), which deletes attachment files before the snackbar appears. The undo action only re-inserts removedNotes, so notes with images/files/audio come back pointing at files that no longer exist.
🤖 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/ModelFolderObserver.kt`
around lines 258 - 269, The undo currently reinserts notes but their attachment
files were already deleted by
baseModel.deleteSelectedBaseNotes()/BaseNoteModel.deleteBaseNotes(...); change
the flow so files are not permanently removed until the Snackbar undo window
passes: either (1) modify BaseNoteModel.deleteBaseNotes to only mark rows
deleted and return the list of attachment file paths (or accept a deferDeletion
flag) and then perform physical file deletion in the Snackbar dismiss callback
when no undo occurred, or (2) capture the attachment file paths before calling
baseModel.deleteSelectedBaseNotes() and, if the user taps Undo (setAction ->
baseModel.saveNotes(removedNotes)), restore those files from a temporary staging
area (or skip deleting them). Update ModelFolderObserver (the positive button
handler and the Snackbar setAction/dismiss logic) to coordinate this deferred
deletion behavior and use the unique symbols
baseModel.deleteSelectedBaseNotes(), BaseNoteModel.deleteBaseNotes(...), and
baseModel.saveNotes(removedNotes) to locate and implement the changes.
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt (1)
276-296:⚠️ Potential issue | 🔴 CriticalDelete attachments only when Undo was not used.
DISMISS_EVENT_ACTIONis the Undo case. Deleting files under that branch means Undo still loses attachments, while timeout/swipe/consecutive dismissals now leak them on disk.Suggested fix
override fun onDismissed( transientBottomBar: Snackbar?, event: Int, ) { - if (event == DISMISS_EVENT_ACTION) { + if (event != DISMISS_EVENT_ACTION) { val attachments = ArrayList<Attachment>() deletedNotes.forEach { note -> attachments.addAll(note.images) attachments.addAll(note.files) attachments.addAll(note.audios)🤖 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/ModelFolderObserver.kt` around lines 276 - 296, The current onDismissed handler deletes attachments when event == DISMISS_EVENT_ACTION (the Undo case); change the logic so attachments are deleted only when the dismiss event is NOT DISMISS_EVENT_ACTION. In the Snackbar.Callback.onDismissed override (referencing DISMISS_EVENT_ACTION, deletedNotes, baseModel.progress, DeleteProgress, and activity.deleteAttachments), invert the condition (or use !=) so the block that collects attachments and calls activity.deleteAttachments(...) runs only when event != DISMISS_EVENT_ACTION, ensuring Undo preserves attachments and only timeout/swipe/other dismissals trigger deletion.
🤖 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/main/ModelFolderObserver.kt`:
- Line 275: The Snackbar Undo currently only calls saveNotes(removedNotes) which
restores rows but not reminders/pinned notifications; create a restore helper
(e.g., restoreDeletedNotes(removedNotes) or reuse an existing restore flow) that
reinserts the notes (saveNotes) and then reactivates them by calling
pinAndScheduleReminders(removedNotes) (the inverse of app.cancelPinAndReminders
used in BaseNoteModel.deleteBaseNotes()), and change the
setAction(R.string.undo) lambda to call that restore helper instead of directly
calling saveNotes(removedNotes).
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 656-660: The moveBaseNotes function currently launches a coroutine
that calls app.moveBaseNotes(...) and only invokes callable on success, so
exceptions inside the coroutine skip the callback and leave loading stuck; wrap
the call in a try/finally (or try/catch then finally) inside the launched
coroutine so that callable?.invoke() is executed in the finally block regardless
of success or failure, referencing moveBaseNotes, app.moveBaseNotes,
viewModelScope.launch and callable?.invoke to locate the change; preserve
Dispatchers.IO and ensure any caught exception is optionally logged/handled
before rethrowing if needed.
- Around line 625-632: pinBaseNotesToStatusBar currently runs DB and
UI/permission work on Dispatchers.IO and captures an Activity directly; change
it to collect ids and create a WeakReference(activity) first, then run
baseNoteDao.updatePinnedToStatus(ids, pinnedToStatusBar) and val updatedNotes =
baseNoteDao.getByIds(ids) on Dispatchers.IO, and afterwards switch to
Dispatchers.Main (withContext or a separate launch on Main) to get the Activity
from the WeakReference and only if non-null call activity.refreshStatusBarPin
for each note; keep references to actionMode.selectedIds,
actionMode.close(true), baseNoteDao.updatePinnedToStatus, baseNoteDao.getByIds
and activity.refreshStatusBarPin to locate the code to modify.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt`:
- Around line 276-296: The current onDismissed handler deletes attachments when
event == DISMISS_EVENT_ACTION (the Undo case); change the logic so attachments
are deleted only when the dismiss event is NOT DISMISS_EVENT_ACTION. In the
Snackbar.Callback.onDismissed override (referencing DISMISS_EVENT_ACTION,
deletedNotes, baseModel.progress, DeleteProgress, and
activity.deleteAttachments), invert the condition (or use !=) so the block that
collects attachments and calls activity.deleteAttachments(...) runs only when
event != DISMISS_EVENT_ACTION, ensuring Undo preserves attachments and only
timeout/swipe/other dismissals trigger deletion.
🪄 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: a73c84ca-33f4-4525-ba14-cea28ccd3538
📒 Files selected for processing (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/BaseNoteModel.kt`:
- Around line 628-635: The code uses with(Dispatchers.IO) inside
viewModelScope.launch which does not change coroutine context, so
baseNoteDao.updatePinnedToStatus and baseNoteDao.getByIds still run on the Main
dispatcher; replace with(Dispatchers.IO) by calling withContext(Dispatchers.IO)
to perform those DB calls on the IO dispatcher, then keep the rest
(updatedNotes.forEach { activity.refreshStatusBarPin(it) }) on the original
coroutine context.
🪄 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: 5c0d6e50-06a8-49d8-a042-0e5f2ac91637
📒 Files selected for processing (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
Show resolved
Hide resolved
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
629-634:⚠️ Potential issue | 🟠 MajorOnly post new status-bar notifications for notes still in
Folder.NOTES.This loop is fine for unpinning because
refreshStatusBarPin()cancels whenisPinnedToStatusis false, but for pinning it will also notify archived/deleted notes. That conflicts withapp/src/main/java/com/philkes/notallyx/data/dao/DaoExtensions.kt:8-21, which treats status-bar pins as active only inFolder.NOTES.Minimal fix
- updatedNotes.forEach { activity.refreshStatusBarPin(it) } + updatedNotes.forEach { note -> + if (!note.isPinnedToStatus || note.folder == Folder.NOTES) { + activity.refreshStatusBarPin(note) + } + }🤖 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/BaseNoteModel.kt` around lines 629 - 634, After updating pins and fetching updatedNotes in BaseNoteModel (the block calling baseNoteDao.updatePinnedToStatus(ids, pinnedToStatusBar) and baseNoteDao.getByIds(ids)), only call activity.refreshStatusBarPin(...) for notes that are actually in Folder.NOTES; filter updatedNotes by note.folder == Folder.NOTES (or the equivalent folder property) before iterating so archived/deleted notes do not produce status-bar notifications when pinned.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
660-664: Consider refactoring to make dispatcher switching explicit.While the current implementation is technically safe—
postValue()is designed to be called from any thread and safely posts updates to the main thread—the suggested refactor improves code clarity by making the dispatcher boundary explicit:Suggested refactor
- viewModelScope.launch(Dispatchers.IO) { - try { - app.moveBaseNotes(baseNoteDao, ids, folder) - } finally { - callable?.invoke() - } - } + viewModelScope.launch { + try { + withContext(Dispatchers.IO) { + app.moveBaseNotes(baseNoteDao, ids, folder) + } + } finally { + callable?.invoke() + } + }This ensures the callback always runs on the default dispatcher (Main), making the threading behavior explicit rather than relying on
postValue()'s thread-safety semantics.🤖 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/BaseNoteModel.kt` around lines 660 - 664, The code launches a coroutine on Dispatchers.IO to call app.moveBaseNotes(...) and then invokes callable?.invoke() in the finally block; make the dispatcher boundary explicit by ensuring heavy work runs on Dispatchers.IO (viewModelScope.launch(Dispatchers.IO) calling app.moveBaseNotes) but switch to Dispatchers.Main before invoking the UI callback so callable?.invoke() runs on Main. Locate the coroutine using viewModelScope.launch(Dispatchers.IO) and the finally block with callable?.invoke(), wrap the callback invocation in a withContext(Dispatchers.Main) (or equivalent dispatcher switch) so the threading intent is explicit and clear.
🤖 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/BaseNoteModel.kt`:
- Around line 692-699: deleteBaseNotes currently fetches notes, closes action
mode and cancels pins/reminders but never removes attached files from disk
before hard-deleting rows, which leaves orphaned files; update deleteBaseNotes
to call app.deleteAttachments(notes) (same call used in
deleteAllTrashedBaseNotes) after fetching the notes and before invoking
baseNoteDao.delete(ids) so attachments are removed on the permanent-delete path
while preserving the existing actionMode.close(false) and
app.cancelPinAndReminders(notes) calls.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 629-634: After updating pins and fetching updatedNotes in
BaseNoteModel (the block calling baseNoteDao.updatePinnedToStatus(ids,
pinnedToStatusBar) and baseNoteDao.getByIds(ids)), only call
activity.refreshStatusBarPin(...) for notes that are actually in Folder.NOTES;
filter updatedNotes by note.folder == Folder.NOTES (or the equivalent folder
property) before iterating so archived/deleted notes do not produce status-bar
notifications when pinned.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 660-664: The code launches a coroutine on Dispatchers.IO to call
app.moveBaseNotes(...) and then invokes callable?.invoke() in the finally block;
make the dispatcher boundary explicit by ensuring heavy work runs on
Dispatchers.IO (viewModelScope.launch(Dispatchers.IO) calling app.moveBaseNotes)
but switch to Dispatchers.Main before invoking the UI callback so
callable?.invoke() runs on Main. Locate the coroutine using
viewModelScope.launch(Dispatchers.IO) and the finally block with
callable?.invoke(), wrap the callback invocation in a
withContext(Dispatchers.Main) (or equivalent dispatcher switch) so the threading
intent is explicit and clear.
🪄 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: c64bc99d-4705-4903-bd43-16bd16b96eef
📒 Files selected for processing (1)
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/com/philkes/notallyx/data/model/ModelExtensions.kt (1)
126-157:⚠️ Potential issue | 🟠 Major
isPinnedToStatusis parsed but not serialized intoJson().The
toBaseNote()function correctly parsesisPinnedToStatusfrom JSON (line 126), but the correspondingtoJson()function (lines 90-114) does not serialize this field. This creates an asymmetry that will cause the pin-to-status-bar state to be lost when notes are exported and re-imported via backup.🐛 Proposed fix to add serialization in `toJson()`
Add the following line in the
toJson()function after the existing field serializations (around line 112):jsonObject.put("reminders", Converters.remindersToJSONArray(reminders)) jsonObject.put("viewMode", viewMode.name) + jsonObject.put("isPinnedToStatus", isPinnedToStatus) return jsonObject.toString(2) }🤖 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/data/model/ModelExtensions.kt` around lines 126 - 157, toBaseNote() reads the isPinnedToStatus flag into BaseNote.pinnedToStatusBar but toJson() never writes it back; update the toJson() implementation to serialize BaseNote.pinnedToStatusBar (the isPinnedToStatus JSON key) so the pin-to-status-bar state round-trips. Locate the toJson() function for BaseNote/notes and add a boolean entry for "isPinnedToStatus" using the BaseNote.pinnedToStatusBar property (ensure the JSON key matches the one parsed by toBaseNote()).app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
678-690:⚠️ Potential issue | 🟠 Major
deleteAll()leaves orphaned attachment files on disk.When clearing all data,
deleteAll()callsdeleteBaseNotes(ids)which only removes database rows and cancels reminders—it does not delete attachment files. In contrast,deleteAllTrashedBaseNotes()properly fetches all image, file, and audio attachments and callsapp.deleteAttachments()before deleting notes. This inconsistency causes orphaned files to remain in storage after the "clear all data" operation completes.🤖 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/BaseNoteModel.kt` around lines 678 - 690, deleteAll() currently deletes DB rows and cancels reminders but does not remove attachment files, causing orphaned files; update deleteAll() to mirror deleteAllTrashedBaseNotes() by loading all attachments for the notes before deleting and calling app.deleteAttachments(...) for image/file/audio attachments (or the same attachment-collection helper used in deleteAllTrashedBaseNotes()) on the IO dispatcher, then proceed to cancel reminders and call deleteBaseNotes(ids); reference functions/variables: deleteAll(), deleteBaseNotes(ids), deleteAllTrashedBaseNotes(), app.deleteAttachments(...), baseNoteDao.getAllIds(), baseNoteDao.getAllReminders() to locate where to add the attachment-fetch-and-delete step.
♻️ Duplicate comments (4)
app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt (1)
125-146:⚠️ Potential issue | 🔴 CriticalInclude
isPinnedToStatusin the ZIP import cursor.Lines 125-144 don't project
isPinnedToStatus, but Line 323 now callsgetColumnIndexOrThrow("isPinnedToStatus"). That makes ZIP import fail as soon as it reads the first note row. If legacy backups are still supported, the parser should also usegetColumnIndex()and fall back tofalsewhen the column is absent.Minimal fix for the current crash
val columns = arrayOf( "id", "type", "folder", "color", "title", "pinned", "timestamp", "modifiedTimestamp", "labels", "SUBSTR(body, 1, ${MAX_BODY_CHAR_LENGTH}) AS body", "spans", "items", "images", "files", "audios", "reminders", "viewMode", + "isPinnedToStatus", )Also applies to: 323-331
🤖 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/backup/ImportExtensions.kt` around lines 125 - 146, The ZIP import cursor projection is missing "isPinnedToStatus" causing getColumnIndexOrThrow at the note-parsing code (where getColumnIndexOrThrow("isPinnedToStatus") is called) to crash; update the columns array used in the database.query for BaseNote to include "isPinnedToStatus" and change the parsing logic that currently calls getColumnIndexOrThrow("isPinnedToStatus") to instead use getColumnIndex("isPinnedToStatus") with a fallback to false when the column index is -1 (to support legacy backups). Ensure you update both the projection near the columns/ database.query and the parsing block that reads the isPinnedToStatus value so missing-column cases default to false.app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt (1)
64-64:⚠️ Potential issue | 🟠 MajorDuplicate pinned notification restoration on boot.
Both
NotallyXApplication.onCreate()andReminderReceiver.BOOT_COMPLETEDcallrestorePinnedNotifications(). SinceApplication.onCreate()runs before theBOOT_COMPLETEDbroadcast is delivered, pinned notifications will be posted twice on device boot.🔧 Suggested fix: Remove from ReminderReceiver
intent.action == Intent.ACTION_BOOT_COMPLETED -> { if (canScheduleExactAlarms) { rescheduleAlarms(context) } restoreRemindersNotifications(context) - restorePinnedNotifications(context) }🤖 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/ReminderReceiver.kt` at line 64, The restorePinnedNotifications(context) call is duplicated: NotallyXApplication.onCreate() already restores pinned notifications before the BOOT_COMPLETED broadcast arrives, so remove the duplicate call from ReminderReceiver's BOOT_COMPLETED handling. Edit ReminderReceiver.onReceive (the branch handling Intent.ACTION_BOOT_COMPLETED / BOOT_COMPLETED) to delete the restorePinnedNotifications(context) invocation while keeping any other boot-specific logic intact so notifications are posted only once.app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt (1)
274-275:⚠️ Potential issue | 🟠 MajorUndo does not restore reminders and status-bar pins.
deleteSelectedBaseNotes()cancels reminders and pins viaapp.cancelPinAndReminders(notes), but the undo action only re-inserts rows withsaveNotes(removedNotes). After undo, scheduled reminders and pinned notifications are lost.🔧 Proposed fix
- setAction(R.string.undo) { baseModel.saveNotes(removedNotes) } + setAction(R.string.undo) { + baseModel.saveNotes(removedNotes) + activity.pinAndScheduleReminders(removedNotes) + }You'll need to import
pinAndScheduleRemindersfromcom.philkes.notallyx.utils.🤖 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/ModelFolderObserver.kt` around lines 274 - 275, The undo action currently only calls baseModel.saveNotes(removedNotes) so reminders and pinned notifications cancelled by app.cancelPinAndReminders(notes) in deleteSelectedBaseNotes() are not restored; update the undo lambda (the setAction in ModelFolderObserver) to, after saving the notes with baseModel.saveNotes(removedNotes), call com.philkes.notallyx.utils.pinAndScheduleReminders(removedNotes) to re-pin notifications and re-schedule reminders, and add the import for pinAndScheduleReminders at the top of the file.app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt (1)
625-636:⚠️ Potential issue | 🟠 MajorActivity reference captured in coroutine may crash on configuration change.
The
activityparameter is captured directly in theviewModelScopecoroutine. If a configuration change (e.g., rotation) occurs before the coroutine completes, the Activity is destroyed while the ViewModel and its coroutine survive, leaving a stale reference. CallingrefreshStatusBarPinon the destroyed Activity will crash.🛡️ Proposed fix using WeakReference
+import java.lang.ref.WeakReference + fun pinBaseNotesToStatusBar(activity: Activity, pinnedToStatusBar: Boolean) { val ids = actionMode.selectedIds.toLongArray() + val activityRef = WeakReference(activity) actionMode.close(true) viewModelScope.launch { val updatedNotes = withContext(Dispatchers.IO) { baseNoteDao.updatePinnedToStatus(ids, pinnedToStatusBar) baseNoteDao.getByIds(ids) } - updatedNotes.forEach { activity.refreshStatusBarPin(it) } + activityRef.get()?.let { act -> + updatedNotes.forEach { act.refreshStatusBarPin(it) } + } } }🤖 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/BaseNoteModel.kt` around lines 625 - 636, The pinBaseNotesToStatusBar function captures the Activity directly into the viewModelScope coroutine which can hold a stale reference across configuration changes; change it to capture a WeakReference(activity) before launching the coroutine (leave actionMode and ids handling unchanged), and inside the coroutine obtain activityRef.get() and only call refreshStatusBarPin(it) when the retrieved Activity is non-null and not finishing/destroyed; this removes the strong Activity reference and prevents calling refreshStatusBarPin on a destroyed Activity while keeping the existing calls to baseNoteDao.updatePinnedToStatus and baseNoteDao.getByIds intact.
🧹 Nitpick comments (6)
app/src/main/java/com/philkes/notallyx/NotallyXApplication.kt (1)
170-174: Optional cleanup: remove redundant pinned-state check in restore loop.
getAllPinnedToStatusNotes()already implies filtering, so theif (note.isPinnedToStatus)guard at Line 172 appears unnecessary.🤖 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/NotallyXApplication.kt` around lines 170 - 174, The restore loop calls getAllPinnedToStatusNotes() which already returns only pinned notes, so remove the redundant if (note.isPinnedToStatus) guard and directly call PinnedNotificationManager.notify(this@NotallyXApplication, note) inside the forEach; update the loop in NotallyXApplication.kt (the block iterating over getAllPinnedToStatusNotes()) to eliminate the unnecessary conditional and keep the notification invocation.app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt (1)
214-218: Consider makingupdatePinnedToStatussuspend for consistency.The
updatePinnedToStatusmethods are notsuspendfunctions, while similar methods likeupdatePinned(line 212) are. While the current callers handle this correctly by running onDispatchers.IO, marking these assuspendwould:
- Maintain consistency with other update methods
- Ensure Room handles threading automatically
- Prevent potential misuse from main thread
♻️ Suggested change
- `@Query`("UPDATE BaseNote SET isPinnedToStatus = :isPinnedToStatus WHERE id = :id") - fun updatePinnedToStatus(id: Long, isPinnedToStatus: Boolean) + `@Query`("UPDATE BaseNote SET isPinnedToStatus = :isPinnedToStatus WHERE id = :id") + suspend fun updatePinnedToStatus(id: Long, isPinnedToStatus: Boolean) - `@Query`("UPDATE BaseNote SET isPinnedToStatus = :isPinnedToStatus WHERE id IN (:ids)") - fun updatePinnedToStatus(ids: LongArray, isPinnedToStatus: Boolean) + `@Query`("UPDATE BaseNote SET isPinnedToStatus = :isPinnedToStatus WHERE id IN (:ids)") + suspend fun updatePinnedToStatus(ids: LongArray, isPinnedToStatus: Boolean)🤖 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/data/dao/BaseNoteDao.kt` around lines 214 - 218, The two DAO methods updatePinnedToStatus in BaseNoteDao should be marked suspend for consistency and to let Room manage threading; update both signatures (the single-id overload and the LongArray overload) to "suspend fun updatePinnedToStatus(...)" keeping the same parameters and return type (Unit), and then run a quick grep of callers to ensure they don't rely on them being non-suspending (update callers to call from a coroutine or suspend context if needed).app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt (2)
153-156: Remove commented-out code.This commented code block appears to be leftover from development. If this logic is no longer needed, please remove it to keep the codebase clean.
🧹 Suggested fix
- // val mostRecentReminder = note.reminders.findLastNotified() ?: return - // if (mostRecentReminder.isNotificationVisible) { - // notify(context, note.id, mostRecentReminder.id, schedule = false) - // }🤖 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/ReminderReceiver.kt` around lines 153 - 156, In ReminderReceiver.kt remove the leftover commented-out block that includes calls to findLastNotified() and notify(...) (the three commented lines referencing mostRecentReminder and isNotificationVisible) to clean up dead code; edit the ReminderReceiver class to delete those commented lines so only active logic remains and run a build to ensure no referenced symbols (mostRecentReminder, findLastNotified, isNotificationVisible) are needed anywhere else.
302-311: Redundant filter:getAllPinnedToStatusNotes()already returns only pinned notes.The query at line 126-128 in
BaseNoteDao.ktincludesWHERE isPinnedToStatus = 1, so the.filter { it.isPinnedToStatus }at line 306 is unnecessary.♻️ Suggested fix
private suspend fun restorePinnedNotifications(context: Context) { val baseNoteDao = getDatabase(context).getBaseNoteDao() val allNotes = baseNoteDao.getAllPinnedToStatusNotes() allNotes - .filter { it.isPinnedToStatus } .forEach { note -> Log.d(TAG, "restorePinnedNotifications: Pinning noteID: ${note.id} to status bar") PinnedNotificationManager.notify(context, note) } }🤖 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/ReminderReceiver.kt` around lines 302 - 311, The restorePinnedNotifications function redundantly filters results already constrained by BaseNoteDao.getAllPinnedToStatusNotes(); remove the .filter { it.isPinnedToStatus } call and iterate directly over allNotes (e.g., allNotes.forEach { note -> ... }) so restorePinnedNotifications uses the DAO result without re-checking isPinnedToStatus.app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt (1)
517-535: Consider simplifyingrefreshStatusBarPinpermission check.The function checks notification permission when
isPinnedToStatusis true, but if a note is already pinned, permission should have been granted during the initial pin action. The permission check here is defensive (handles permission revocation after pinning), but the structure could be simplified:♻️ Simplified alternative
fun Activity.refreshStatusBarPin(note: BaseNote) { - val refresh = { - if (note.isPinnedToStatus) { - PinnedNotificationManager.notify(this, note) - } else { - PinnedNotificationManager.cancel(this, note.id) - } - } if (note.isPinnedToStatus) { - checkNotificationPermission( - REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS, - alsoCheckAlarmPermission = false, - ) { - refresh() - } + // Permission already verified during pin action; notify silently fails if revoked + PinnedNotificationManager.notify(this, note) } else { - refresh() + PinnedNotificationManager.cancel(this, note.id) } }The current defensive approach is valid if you want to re-prompt for permission when it was revoked after initial grant.
🤖 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/NoteActionHandler.kt` around lines 517 - 535, The refreshStatusBarPin function currently defines a refresh lambda and then branches to either call checkNotificationPermission(...) { refresh() } when note.isPinnedToStatus or refresh() otherwise; simplify by removing the refresh lambda and inlining the actions: when note.isPinnedToStatus call checkNotificationPermission(REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS, alsoCheckAlarmPermission = false) { PinnedNotificationManager.notify(this, note) } and otherwise directly call PinnedNotificationManager.cancel(this, note.id). Keep the same permission request symbol (REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS) and methods (checkNotificationPermission, PinnedNotificationManager.notify, PinnedNotificationManager.cancel) to locate the change.app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.kt (1)
26-38: Remove duplicate text computation.
contentTextandbigTextare computed with identical logic. Extract to a single variable.♻️ Proposed fix
- val contentText = + val noteText = if (note.type == com.philkes.notallyx.data.model.Type.LIST) { note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } } else { note.body } - val bigText = - if (note.type == com.philkes.notallyx.data.model.Type.LIST) { - note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } - } else { - note.body - } val notification = NotificationCompat.Builder(this, channelId) .apply { setSubText(subtext) setSmallIcon(iconResId) setCategory(NotificationCompat.CATEGORY_REMINDER) setContentTitle(note.title.ifEmpty { getString(R.string.note) }) - setContentText(contentText) - setStyle(NotificationCompat.BigTextStyle().bigText(bigText)) + setContentText(noteText) + setStyle(NotificationCompat.BigTextStyle().bigText(noteText))🤖 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/RemindersUtils.kt` around lines 26 - 38, The logic computing the same string for contentText and bigText is duplicated; compute it once (e.g., val computedText or noteText) by checking note.type == com.philkes.notallyx.data.model.Type.LIST and using note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } or note.body, then assign contentText = computedText and bigText = computedText so both variables reuse the single computed value (update references to contentText and bigText in RemindersUtils.kt accordingly).
🤖 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/data/model/BaseNote.kt`:
- Around line 64-69: The equalContents(other: BaseNote?) method in BaseNote
fails to compare the note type, so notes that only differ by type (Type.NOTE vs
Type.LIST) are treated as equal and cause incorrect notification intents; update
equalContents to include a comparison of the type property (e.g., add type ==
other.type) alongside title, body, spans and items so type changes are detected
and notifications get rebuilt appropriately.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt`:
- Around line 276-299: The dismissal branch is inverted: inside
Snackbar.Callback.onDismissed you currently delete attachments when event ==
DISMISS_EVENT_ACTION (which means the user tapped "Undo"); change the condition
so attachments are deleted only when the dismissal was NOT the action/undo.
Update the onDismissed check (in the anonymous Snackbar.Callback implementation)
to run the attachment collection and call activity.deleteAttachments(...) only
when event != DISMISS_EVENT_ACTION (i.e., when the user did not undo), leaving
the rest of the logic (deletedNotes, attachments list, baseModel.progress =
DeleteProgress(...)) intact.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.kt`:
- Around line 83-102: Change createDeleteReminderIntent to return a non-nullable
PendingIntent and guard against the case where both reminderId and
requestCodePostfix are null: validate at the start of
Context.createDeleteReminderIntent that if reminderId == null then
requestCodePostfix is not null (throw IllegalArgumentException or similar with a
clear message), then build the request-code string using either reminderId or
requestCodePostfix and call PendingIntent.getBroadcast as before; update the
function signature to return PendingIntent (not PendingIntent?) and reference
EXTRA_NOTE_ID, EXTRA_REMINDER_ID and ReminderReceiver::class.java unchanged.
In `@app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt`:
- Around line 462-468: The PendingIntent returned by getOpenNotePendingIntent
currently uses a fixed requestCode (0), causing different note notifications to
collapse into the same PendingIntent; update getOpenNotePendingIntent to
generate a unique requestCode per note (e.g., derive from noteId and noteType —
combine noteId.hashCode() and noteType.ordinal or type.hashCode()) and pass that
as the second argument to PendingIntent.getActivity so each note's PendingIntent
is distinct and notifications won't reuse the same intent.
In `@app/src/main/res/drawable/notification_unpin.xml`:
- Around line 10-21: The vector path in notification_unpin.xml has a missing
segment in the android:pathData causing bad rendering; update the pathData in
notification_unpin.xml (the first <path> entry) to include the missing segment
`200L280,200L280,514L234,560` between the coordinates after `387,` and before
`560L497,` or simply regenerate the entire icon from the original vector/source
asset (matching notification_pin.xml's corresponding path) so the geometry is
correct and the icon renders properly.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.kt`:
- Around line 126-157: toBaseNote() reads the isPinnedToStatus flag into
BaseNote.pinnedToStatusBar but toJson() never writes it back; update the
toJson() implementation to serialize BaseNote.pinnedToStatusBar (the
isPinnedToStatus JSON key) so the pin-to-status-bar state round-trips. Locate
the toJson() function for BaseNote/notes and add a boolean entry for
"isPinnedToStatus" using the BaseNote.pinnedToStatusBar property (ensure the
JSON key matches the one parsed by toBaseNote()).
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 678-690: deleteAll() currently deletes DB rows and cancels
reminders but does not remove attachment files, causing orphaned files; update
deleteAll() to mirror deleteAllTrashedBaseNotes() by loading all attachments for
the notes before deleting and calling app.deleteAttachments(...) for
image/file/audio attachments (or the same attachment-collection helper used in
deleteAllTrashedBaseNotes()) on the IO dispatcher, then proceed to cancel
reminders and call deleteBaseNotes(ids); reference functions/variables:
deleteAll(), deleteBaseNotes(ids), deleteAllTrashedBaseNotes(),
app.deleteAttachments(...), baseNoteDao.getAllIds(),
baseNoteDao.getAllReminders() to locate where to add the
attachment-fetch-and-delete step.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt`:
- Around line 274-275: The undo action currently only calls
baseModel.saveNotes(removedNotes) so reminders and pinned notifications
cancelled by app.cancelPinAndReminders(notes) in deleteSelectedBaseNotes() are
not restored; update the undo lambda (the setAction in ModelFolderObserver) to,
after saving the notes with baseModel.saveNotes(removedNotes), call
com.philkes.notallyx.utils.pinAndScheduleReminders(removedNotes) to re-pin
notifications and re-schedule reminders, and add the import for
pinAndScheduleReminders at the top of the file.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt`:
- Line 64: The restorePinnedNotifications(context) call is duplicated:
NotallyXApplication.onCreate() already restores pinned notifications before the
BOOT_COMPLETED broadcast arrives, so remove the duplicate call from
ReminderReceiver's BOOT_COMPLETED handling. Edit ReminderReceiver.onReceive (the
branch handling Intent.ACTION_BOOT_COMPLETED / BOOT_COMPLETED) to delete the
restorePinnedNotifications(context) invocation while keeping any other
boot-specific logic intact so notifications are posted only once.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 625-636: The pinBaseNotesToStatusBar function captures the
Activity directly into the viewModelScope coroutine which can hold a stale
reference across configuration changes; change it to capture a
WeakReference(activity) before launching the coroutine (leave actionMode and ids
handling unchanged), and inside the coroutine obtain activityRef.get() and only
call refreshStatusBarPin(it) when the retrieved Activity is non-null and not
finishing/destroyed; this removes the strong Activity reference and prevents
calling refreshStatusBarPin on a destroyed Activity while keeping the existing
calls to baseNoteDao.updatePinnedToStatus and baseNoteDao.getByIds intact.
In `@app/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.kt`:
- Around line 125-146: The ZIP import cursor projection is missing
"isPinnedToStatus" causing getColumnIndexOrThrow at the note-parsing code (where
getColumnIndexOrThrow("isPinnedToStatus") is called) to crash; update the
columns array used in the database.query for BaseNote to include
"isPinnedToStatus" and change the parsing logic that currently calls
getColumnIndexOrThrow("isPinnedToStatus") to instead use
getColumnIndex("isPinnedToStatus") with a fallback to false when the column
index is -1 (to support legacy backups). Ensure you update both the projection
near the columns/ database.query and the parsing block that reads the
isPinnedToStatus value so missing-column cases default to false.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt`:
- Around line 214-218: The two DAO methods updatePinnedToStatus in BaseNoteDao
should be marked suspend for consistency and to let Room manage threading;
update both signatures (the single-id overload and the LongArray overload) to
"suspend fun updatePinnedToStatus(...)" keeping the same parameters and return
type (Unit), and then run a quick grep of callers to ensure they don't rely on
them being non-suspending (update callers to call from a coroutine or suspend
context if needed).
In `@app/src/main/java/com/philkes/notallyx/NotallyXApplication.kt`:
- Around line 170-174: The restore loop calls getAllPinnedToStatusNotes() which
already returns only pinned notes, so remove the redundant if
(note.isPinnedToStatus) guard and directly call
PinnedNotificationManager.notify(this@NotallyXApplication, note) inside the
forEach; update the loop in NotallyXApplication.kt (the block iterating over
getAllPinnedToStatusNotes()) to eliminate the unnecessary conditional and keep
the notification invocation.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.kt`:
- Around line 517-535: The refreshStatusBarPin function currently defines a
refresh lambda and then branches to either call checkNotificationPermission(...)
{ refresh() } when note.isPinnedToStatus or refresh() otherwise; simplify by
removing the refresh lambda and inlining the actions: when note.isPinnedToStatus
call checkNotificationPermission(REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS,
alsoCheckAlarmPermission = false) { PinnedNotificationManager.notify(this, note)
} and otherwise directly call PinnedNotificationManager.cancel(this, note.id).
Keep the same permission request symbol
(REQUEST_NOTIFICATION_PERMISSION_PIN_TO_STATUS) and methods
(checkNotificationPermission, PinnedNotificationManager.notify,
PinnedNotificationManager.cancel) to locate the change.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.kt`:
- Around line 153-156: In ReminderReceiver.kt remove the leftover commented-out
block that includes calls to findLastNotified() and notify(...) (the three
commented lines referencing mostRecentReminder and isNotificationVisible) to
clean up dead code; edit the ReminderReceiver class to delete those commented
lines so only active logic remains and run a build to ensure no referenced
symbols (mostRecentReminder, findLastNotified, isNotificationVisible) are needed
anywhere else.
- Around line 302-311: The restorePinnedNotifications function redundantly
filters results already constrained by BaseNoteDao.getAllPinnedToStatusNotes();
remove the .filter { it.isPinnedToStatus } call and iterate directly over
allNotes (e.g., allNotes.forEach { note -> ... }) so restorePinnedNotifications
uses the DAO result without re-checking isPinnedToStatus.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.kt`:
- Around line 26-38: The logic computing the same string for contentText and
bigText is duplicated; compute it once (e.g., val computedText or noteText) by
checking note.type == com.philkes.notallyx.data.model.Type.LIST and using
note.items.joinToString("\n") { (if (it.checked) "✅ " else "🔳 ") + it.body } or
note.body, then assign contentText = computedText and bigText = computedText so
both variables reuse the single computed value (update references to contentText
and bigText in RemindersUtils.kt accordingly).
🪄 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: e05e0a13-b001-4993-8da1-91c13b99a0dd
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (44)
app/schemas/com.philkes.notallyx.data.NotallyDatabase/10.jsonapp/src/main/AndroidManifest.xmlapp/src/main/java/com/philkes/notallyx/NotallyXApplication.ktapp/src/main/java/com/philkes/notallyx/data/NotallyDatabase.ktapp/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.ktapp/src/main/java/com/philkes/notallyx/data/dao/DaoExtensions.ktapp/src/main/java/com/philkes/notallyx/data/imports/evernote/EvernoteImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/google/GoogleKeepImporter.ktapp/src/main/java/com/philkes/notallyx/data/imports/txt/PlainTextImporter.ktapp/src/main/java/com/philkes/notallyx/data/model/BaseNote.ktapp/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/NoteActionHandler.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/ReminderReceiver.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/reminders/RemindersUtils.ktapp/src/main/java/com/philkes/notallyx/presentation/view/note/action/MoreNoteBottomSheet.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/NotallyModel.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/Preference.ktapp/src/main/java/com/philkes/notallyx/utils/AlarmExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/PinnedNotificationManager.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/XmlParserExtensions.ktapp/src/main/res/drawable/notification_pin.xmlapp/src/main/res/drawable/notification_unpin.xmlapp/src/main/res/drawable/pinboard.xmlapp/src/main/res/drawable/pinboard_filled.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
app/src/main/java/com/philkes/notallyx/presentation/activity/main/ModelFolderObserver.kt
Show resolved
Hide resolved
Comment on lines
+83
to
+102
| fun Context.createDeleteReminderIntent( | ||
| noteId: Long, | ||
| reminderId: Long?, | ||
| requestCodePostfix: String? = null, | ||
| ): PendingIntent? { | ||
| val deleteIntent = | ||
| Intent(this, ReminderReceiver::class.java).apply { | ||
| action = | ||
| if (reminderId != null) ReminderReceiver.ACTION_NOTIFICATION_DISMISSED | ||
| else ReminderReceiver.ACTION_PINNED_NOTIFICATION_DISMISSED | ||
| putExtra(EXTRA_NOTE_ID, noteId) | ||
| reminderId?.let { putExtra(EXTRA_REMINDER_ID, it) } | ||
| } | ||
| return PendingIntent.getBroadcast( | ||
| this, | ||
| "$noteId-${reminderId ?: requestCodePostfix!!}".hashCode(), | ||
| deleteIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Return type should be non-nullable; add guard against null inputs.
The function always returns a PendingIntent from getBroadcast, yet the return type is PendingIntent?. More critically, if both reminderId and requestCodePostfix are null, line 98 will throw NullPointerException on the !! operator.
🛡️ Proposed fix
fun Context.createDeleteReminderIntent(
noteId: Long,
reminderId: Long?,
requestCodePostfix: String? = null,
-): PendingIntent? {
+): PendingIntent {
+ require(reminderId != null || requestCodePostfix != null) {
+ "Either reminderId or requestCodePostfix must be non-null"
+ }
val deleteIntent =
Intent(this, ReminderReceiver::class.java).apply {📝 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
| fun Context.createDeleteReminderIntent( | |
| noteId: Long, | |
| reminderId: Long?, | |
| requestCodePostfix: String? = null, | |
| ): PendingIntent? { | |
| val deleteIntent = | |
| Intent(this, ReminderReceiver::class.java).apply { | |
| action = | |
| if (reminderId != null) ReminderReceiver.ACTION_NOTIFICATION_DISMISSED | |
| else ReminderReceiver.ACTION_PINNED_NOTIFICATION_DISMISSED | |
| putExtra(EXTRA_NOTE_ID, noteId) | |
| reminderId?.let { putExtra(EXTRA_REMINDER_ID, it) } | |
| } | |
| return PendingIntent.getBroadcast( | |
| this, | |
| "$noteId-${reminderId ?: requestCodePostfix!!}".hashCode(), | |
| deleteIntent, | |
| PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE, | |
| ) | |
| } | |
| fun Context.createDeleteReminderIntent( | |
| noteId: Long, | |
| reminderId: Long?, | |
| requestCodePostfix: String? = null, | |
| ): PendingIntent { | |
| require(reminderId != null || requestCodePostfix != null) { | |
| "Either reminderId or requestCodePostfix must be non-null" | |
| } | |
| val deleteIntent = | |
| Intent(this, ReminderReceiver::class.java).apply { | |
| action = | |
| if (reminderId != null) ReminderReceiver.ACTION_NOTIFICATION_DISMISSED | |
| else ReminderReceiver.ACTION_PINNED_NOTIFICATION_DISMISSED | |
| putExtra(EXTRA_NOTE_ID, noteId) | |
| reminderId?.let { putExtra(EXTRA_REMINDER_ID, it) } | |
| } | |
| return PendingIntent.getBroadcast( | |
| this, | |
| "$noteId-${reminderId ?: requestCodePostfix!!}".hashCode(), | |
| deleteIntent, | |
| PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE, | |
| ) | |
| } |
🤖 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/RemindersUtils.kt`
around lines 83 - 102, Change createDeleteReminderIntent to return a
non-nullable PendingIntent and guard against the case where both reminderId and
requestCodePostfix are null: validate at the start of
Context.createDeleteReminderIntent that if reminderId == null then
requestCodePostfix is not null (throw IllegalArgumentException or similar with a
clear message), then build the request-code string using either reminderId or
requestCodePostfix and call PendingIntent.getBroadcast as before; update the
function signature to return PendingIntent (not PendingIntent?) and reference
EXTRA_NOTE_ID, EXTRA_REMINDER_ID and ReminderReceiver::class.java unchanged.
Comment on lines
+10
to
+21
| android:pathData= | ||
| "M360, | ||
| 920L320,880L320, | ||
| 640L120,640L120, | ||
| 560L200,480L200, | ||
| 200L160,200L160, | ||
| 120L445,120Q427, | ||
| 137 412.5,157Q398, 177 387, | ||
| 560L497,560Q521, | ||
| 574 546.5,583.5Q572, | ||
| 593 600,597L600, | ||
| 640L400,640L400,880L360,920Z" /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import xml.etree.ElementTree as ET
ANDROID_NS = "{http://schemas.android.com/apk/res/android}"
files = [
"app/src/main/res/drawable/notification_pin.xml",
"app/src/main/res/drawable/notification_unpin.xml",
]
def first_path_data(path):
root = ET.parse(path).getroot()
first_path = root.findall("path")[0]
return " ".join(first_path.attrib[f"{ANDROID_NS}pathData"].split())
pin = first_path_data(files[0])
unpin = first_path_data(files[1])
print("PIN :", pin)
print("UNPIN :", unpin)
print("\nPIN contains segment '200L280,200L280,514L234,560':", "200L280,200L280,514L234,560" in pin)
print("UNPIN contains segment '200L280,200L280,514L234,560':", "200L280,200L280,514L234,560" in unpin)
PYRepository: Crustack/NotallyX
Length of output: 613
Regenerate icon from source asset—the pathData is missing a segment.
The first pathData in notification_unpin.xml is missing the segment 200L280,200L280,514L234,560 that exists in notification_pin.xml. The unpin path jumps directly from 387, to 560L497, instead of following the complete geometry. This will cause the icon to render incorrectly. Unwrap the pathData or regenerate it from the vector source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/drawable/notification_unpin.xml` around lines 10 - 21, The
vector path in notification_unpin.xml has a missing segment in the
android:pathData causing bad rendering; update the pathData in
notification_unpin.xml (the first <path> entry) to include the missing segment
`200L280,200L280,514L234,560` between the coordinates after `387,` and before
`560L497,` or simply regenerate the entire icon from the original vector/source
asset (matching notification_pin.xml's corresponding path) so the geometry is
correct and the icon renders properly.
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 #880
Closes #914
notallyx_issues_880.webm
Summary by CodeRabbit
New Features
Improvements
Localization
UI