Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request implements manual note sorting by introducing a database Changes
Sequence DiagramsequenceDiagram
participant User
participant Fragment as NotallyFragment
participant Adapter as BaseNoteAdapter
participant ViewModel as BaseNoteModel
participant DAO as BaseNoteDao
participant DB as Database
User->>Fragment: Drag note to new position
Fragment->>Adapter: onItemMove(fromPos, toPos)
Adapter->>Adapter: Swap items in MutableListContainer
Adapter->>Adapter: updateSortIndices()
Adapter->>ViewModel: updateNotesSortIndices(idToIdx)
ViewModel->>DAO: updateSortIndices(idToIdx)
DAO->>DB: INSERT/UPDATE sortIdx values
DB-->>DAO: Confirm
DAO-->>ViewModel: Complete
ViewModel-->>Adapter: Coroutine completes
Adapter->>Fragment: notifyItemMoved()
Fragment->>User: Display reordered note
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/res/layout/dialog_notes_sort.xml (1)
19-28:⚠️ Potential issue | 🟡 MinorLabel visibility not toggled alongside the RadioGroup.
The
NotesSortDirectionRadioGroupLabelTextView is now exposed with an ID, but inPreferenceBindingExtensions.kt(lines 162-167), onlyNotesSortDirectionRadioGroupvisibility is toggled when manual sort is selected. The label ("Sort Direction") would remain visible even when the direction picker is hidden, creating a confusing UI state.Consider also toggling the label's visibility:
🔧 Suggested fix in PreferenceBindingExtensions.kt
layout.NotesSortDirectionRadioGroup.isVisible = (layout.NotesSortByRadioGroup.checkedTag() as? NotesSortBy)?.isManualSort?.not() ?: true + layout.NotesSortDirectionRadioGroupLabel.isVisible = + layout.NotesSortDirectionRadioGroup.isVisible layout.NotesSortByRadioGroup.setOnCheckedChangeListener { group, checkedId -> val selectedSortBy = group.checkedTag() as? NotesSortBy layout.NotesSortDirectionRadioGroup.isVisible = selectedSortBy?.isManualSort?.not() ?: true + layout.NotesSortDirectionRadioGroupLabel.isVisible = + layout.NotesSortDirectionRadioGroup.isVisible }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/layout/dialog_notes_sort.xml` around lines 19 - 28, The TextView with id NotesSortDirectionRadioGroupLabel is not being hidden when the NotesSortDirectionRadioGroup is toggled in the binding code; update the toggle in the PreferenceBindingExtensions code that currently sets visibility for NotesSortDirectionRadioGroup (the same block around where manual sort is handled) to also set the visibility of NotesSortDirectionRadioGroupLabel to the same value (use the same isVisible/visibility logic or helper used for the RadioGroup so the label is hidden/shown in sync). Ensure you reference the existing NotesSortDirectionRadioGroupLabel and NotesSortDirectionRadioGroup view IDs and apply identical visibility changes in that handler.app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt (1)
82-109:⚠️ Potential issue | 🔴 CriticalBulk
insertSafe()drops the sanitized notes.Line 90 produces a possibly truncated
note, but Line 109 still insertsbaseNotes. Oversized imports/batch saves will therefore be reported as truncated without actually persisting the truncated copies. Build the insert list fromnoteand pass that list toinsert(...).Proposed fix
suspend fun insertSafe(context: ContextWrapper, baseNotes: List<BaseNote>): List<Long> { val maxSortIdx = getMaxSortIdx() ?: 0 + val notesToInsert = mutableListOf<BaseNote>() val truncatedNotes = mutableListOf<BaseNote>() var truncatedCharacterSize = 0 baseNotes.forEachIndexed { index, baseNote -> if (baseNote.sortIdx == null) { baseNote.sortIdx = (maxSortIdx + 1 + index) } val (truncated, note) = baseNote.truncated() + notesToInsert.add(note) if (truncated) { truncatedCharacterSize += baseNote.body.length truncatedNotes.add(note) } } if (truncatedNotes.isNotEmpty()) { context.log( TAG, "${truncatedNotes.size} Notes are too big to save, they were truncated to $truncatedCharacterSize characters", ) context.showToast( context.getQuantityString( R.plurals.notes_too_big_truncating, truncatedNotes.size, truncatedCharacterSize, ) ) } - return insert(baseNotes) + return insert(notesToInsert) }🤖 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 82 - 109, The insertSafe function currently detects oversized notes via BaseNote.truncated() and accumulates truncated versions in truncatedNotes but still calls insert(baseNotes), so the sanitized/truncated note copies are never persisted; change insertSafe to build a new list (e.g., insertList) where for each baseNote you append the truncated note returned by baseNote.truncated() when truncated (or the original/baseNote when not truncated), use that list for the final insert(...) call instead of baseNotes, and keep the existing logging/showToast behavior using truncatedNotes/truncatedCharacterSize; update references to truncated, note, truncatedNotes, and the insert(...) call accordingly.app/src/main/java/com/philkes/notallyx/data/model/BaseNote.kt (1)
43-88:⚠️ Potential issue | 🟠 MajorKeep
sortIdxout ofBaseNotecontent equality.
app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteAdapter.ktLines 104-109 useBaseNote.equals()forareContentsTheSame(), and Lines 347-351 rewrite every note’ssortIdxduring drag reorder. With Line 65 now participating in equality, each move is treated as a content update as well, which forces rebinds and fights the move animations this feature depends on. KeepsortIdxout ofequals()/hashCode(), or make the adapter’s content comparison ignore 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/data/model/BaseNote.kt` around lines 43 - 88, BaseNote currently includes sortIdx in content equality which causes drag-reorder to be treated as a content change; update BaseNote.equals(other: Any?) and BaseNote.hashCode() to ignore sortIdx (remove the if (sortIdx != other.sortIdx) check from equals and stop adding sortIdx into the hash calculation), or alternatively make BaseNoteAdapter.areContentsTheSame(...) explicitly ignore sortIdx; prefer removing sortIdx from BaseNote.equals/hashCode by deleting its comparison and its contribution to the hash so reorder updates no longer trigger content rebinds.
🤖 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/ActionModeBinding.kt`:
- Around line 45-50: The OnBackPressedCallback instance actionModeCancelCallback
is never added to the back-press dispatcher so toggling its isEnabled is
ineffective; either remove the unused actionModeCancelCallback and its isEnabled
toggles, or register it with the dispatcher. To fix by registering, call
context.onBackPressedDispatcher.addCallback(lifecycleOwner,
actionModeCancelCallback) (or use the appropriate activity/fragment
onBackPressedDispatcher) when the binding is created so the callback will be
invoked, and keep using isEnabled to toggle it; if you choose removal, delete
actionModeCancelCallback and any code that sets its isEnabled (e.g., where you
enable/disable it) and ensure baseModel.actionMode.close(true) is invoked via
another registered handler.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/NotallyFragment.kt`:
- Around line 263-287: The moveSelectedNotesUp/moveSelectedNotesDown handlers
(and any code that calls updateDatabaseSortIndices/updateNotesSortIndices) are
persisting order using notesAdapter.currentList which may be filtered; instead
either skip persisting when the list is filtered (only allow manual reorder on
full folder view) or build the new sort-index map against the unfiltered/full
dataset before calling updateNotesSortIndices/updateDatabaseSortIndices.
Concretely: detect a filtered state (e.g., compare notesAdapter.currentList size
vs the full notes list or use a filter flag) inside
moveSelectedNotesUp/moveSelectedNotesDown and do not call the DB update if
filtered, or construct an ID->newIndex map by iterating the full notes
collection and applying the new positions from the visible list while preserving
relative positions of hidden items, then pass that map into
updateNotesSortIndices/updateDatabaseSortIndices so hidden notes’ sortIdx remain
consistent.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`:
- Around line 162-168: The visibility toggle only updates
NotesSortDirectionRadioGroup but not its label; update the initial visibility
and the listener to also set layout.NotesSortDirectionRadioGroupLabel.isVisible
to the same boolean you compute (use the same expression as for
NotesSortDirectionRadioGroup), i.e. read selected sort via
layout.NotesSortByRadioGroup.checkedTag() as? NotesSortBy and set both
layout.NotesSortDirectionRadioGroup.isVisible and
layout.NotesSortDirectionRadioGroupLabel.isVisible accordingly in the initial
assignment and inside the setOnCheckedChangeListener callback so the label
hides/shows in sync with the direction radio group.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.kt`:
- Around line 613-618: The currentFragment() helper incorrectly uses
childFragmentManager.fragments.firstOrNull(), which can return stale fragments;
update it to resolve the displayed destination via the NavHost's
primaryNavigationFragment instead: locate the call in currentFragment() (and any
code paths like prepareNewNoteIntent() or manual-sort actions that rely on it),
replace the childFragmentManager.fragments.firstOrNull() lookup with
childFragmentManager.primaryNavigationFragment, and cast that result to
NotallyFragment (as? NotallyFragment) so the app always targets the actual
current fragment shown by the Navigation component.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/RecordAudioActivity.kt`:
- Around line 151-166: The READY branch currently only updates buttons and can
leave Timer running from a prior RECORDING state; update the Status.READY
handling to explicitly stop and reset the chronometer (use the same Timer
control used elsewhere, e.g., call Timer.stop() and reset its base/elapsed
state) in addition to setting Stop.isEnabled = false and
Main.setText(R.string.start), and apply the same stop+reset logic in the empty
else fallback so null or unexpected service.status.value also clears the UI
timer.
In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt`:
- Around line 401-403: RadioGroup.checkedTag() currently dereferences
findViewById(...) and RadioButton.tag unsafely which causes NPE when
checkedRadioButtonId == -1 or tag is null; change the function to safely handle
those cases by returning a nullable type (e.g., Any?) or throwing a clear
IllegalStateException with context, and update all caller sites using unsafe
"as" casts to handle null or perform safe casting; specifically modify
RadioGroup.checkedTag() to check checkedRadioButtonId != -1, null-check the
result of findViewById<RadioButton>(...), and return the nullable tag (or throw)
so callers in MainActivity and PreferenceBindingExtensions replace "as" with
safe casts or null-handling.
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteAdapter.kt`:
- Around line 175-177: Remove the verbose debug log that prints user search
input in setSearchKeyword; locate the Log.d("SearchResult", "keyword: $keyword")
call inside the setSearchKeyword function and delete it (or replace it with a
non-PII-safe log such as a flag/length check or obfuscated/hashed value),
ensuring only non-sensitive information or no logging of the raw keyword remains
while leaving the rest of the setSearchKeyword logic intact.
- Around line 272-345: In moveSelectedNotesUp and moveSelectedNotesDown, prevent
moving a selected BaseNote across section boundaries by checking the adjacent
item and pinned-state before calling (list as MutableListContainer).move: for
each candidate index, fetch the neighboring item (index-1 for up, index+1 for
down) and skip the move if that neighbor is a Header or if its pinned state
differs from the selected item's pinned state; use the original items list to
inspect neighbors and the BaseNote.pinned (or equivalent) flag and only call
move when both adjacent-item and pinned-state guards pass, preserving all other
logic (selectedIndices iteration, moved flag, computing new positions,
updateSortIndices).
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 660-672: The duplicateNotes() flow currently deep-copies notes and
calls baseNoteDao.insert(copies) causing duplicates to inherit the original
sortIdx; update duplicateNotes() so each copied note resets its sortIdx (e.g.,
set sortIdx = null or a sentinel so ordering logic will assign a new index) when
calling deepCopy().copy(...), and replace the direct baseNoteDao.insert(copies)
call with insertSafe(app, copies) so inserts go through the safe insertion path
that assigns proper sort indices; reference duplicateNotes(),
deepCopy().copy(...), baseNoteDao.insert, and insertSafe(app, copies) when
making this change.
---
Outside diff comments:
In `@app/src/main/java/com/philkes/notallyx/data/dao/BaseNoteDao.kt`:
- Around line 82-109: The insertSafe function currently detects oversized notes
via BaseNote.truncated() and accumulates truncated versions in truncatedNotes
but still calls insert(baseNotes), so the sanitized/truncated note copies are
never persisted; change insertSafe to build a new list (e.g., insertList) where
for each baseNote you append the truncated note returned by baseNote.truncated()
when truncated (or the original/baseNote when not truncated), use that list for
the final insert(...) call instead of baseNotes, and keep the existing
logging/showToast behavior using truncatedNotes/truncatedCharacterSize; update
references to truncated, note, truncatedNotes, and the insert(...) call
accordingly.
In `@app/src/main/java/com/philkes/notallyx/data/model/BaseNote.kt`:
- Around line 43-88: BaseNote currently includes sortIdx in content equality
which causes drag-reorder to be treated as a content change; update
BaseNote.equals(other: Any?) and BaseNote.hashCode() to ignore sortIdx (remove
the if (sortIdx != other.sortIdx) check from equals and stop adding sortIdx into
the hash calculation), or alternatively make
BaseNoteAdapter.areContentsTheSame(...) explicitly ignore sortIdx; prefer
removing sortIdx from BaseNote.equals/hashCode by deleting its comparison and
its contribution to the hash so reorder updates no longer trigger content
rebinds.
In `@app/src/main/res/layout/dialog_notes_sort.xml`:
- Around line 19-28: The TextView with id NotesSortDirectionRadioGroupLabel is
not being hidden when the NotesSortDirectionRadioGroup is toggled in the binding
code; update the toggle in the PreferenceBindingExtensions code that currently
sets visibility for NotesSortDirectionRadioGroup (the same block around where
manual sort is handled) to also set the visibility of
NotesSortDirectionRadioGroupLabel to the same value (use the same
isVisible/visibility logic or helper used for the RadioGroup so the label is
hidden/shown in sync). Ensure you reference the existing
NotesSortDirectionRadioGroupLabel and NotesSortDirectionRadioGroup view IDs and
apply identical visibility changes in that handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3199f775-6ef9-45c1-adb2-0fb50178dcf9
📒 Files selected for processing (26)
TRANSLATIONS.mdapp/schemas/com.philkes.notallyx.data.NotallyDatabase/10.jsonapp/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/model/BaseNote.ktapp/src/main/java/com/philkes/notallyx/data/model/ModelExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/ActionModeBinding.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/NotallyFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/RecordAudioActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/sorting/BaseNoteManualSort.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/NotesSorting.ktapp/src/main/java/com/philkes/notallyx/utils/ActionMode.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/res/drawable/sort.xmlapp/src/main/res/layout/activity_main.xmlapp/src/main/res/layout/dialog_notes_sort.xmlapp/src/main/res/layout/fragment_settings.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/values/themes.xml
💤 Files with no reviewable changes (3)
- app/src/main/res/layout/fragment_settings.xml
- app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt
- app/src/main/res/values/themes.xml
Comment on lines
+45
to
+50
| private val actionModeCancelCallback = | ||
| object : OnBackPressedCallback(true) { | ||
| override fun handleOnBackPressed() { | ||
| baseModel.actionMode.close(true) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does an AndroidX OnBackPressedCallbackhave any effect before it is registered with anOnBackPressedDispatcher?
💡 Result:
No. An OnBackPressedCallback does not intercept or receive system Back events until it is added/registered with an OnBackPressedDispatcher (e.g., via addCallback(...)). Before registration it’s just an object holding state (like isEnabled), and the dispatcher only considers enabled callbacks that are registered when deciding whether/what to dispatch on Back. [1], [2]
🏁 Script executed:
cd /tmp && find . -name "ActionModeBinding.kt" 2>/dev/null | head -1Repository: Crustack/NotallyX
Length of output: 43
🏁 Script executed:
fd "ActionModeBinding.kt"Repository: Crustack/NotallyX
Length of output: 147
🏁 Script executed:
find . -type f -name "ActionModeBinding.kt" 2>/dev/nullRepository: Crustack/NotallyX
Length of output: 149
🏁 Script executed:
# Search for ActionModeBinding.kt and read its full contents
fd "ActionModeBinding.kt" --exec cat -n {} \;Repository: Crustack/NotallyX
Length of output: 14530
🏁 Script executed:
# Also search for any references to actionModeCancelCallback to see where it's used
rg "actionModeCancelCallback" -B 2 -A 2Repository: Crustack/NotallyX
Length of output: 6866
🏁 Script executed:
# Search for addCallback or registerCallback patterns in relation to this callback
rg "addCallback|registerCallback" -B 2 -A 2Repository: Crustack/NotallyX
Length of output: 50373
Remove the unused callback or register it with the dispatcher.
actionModeCancelCallback is enabled and disabled in this class (line 74), but it is never registered with context.onBackPressedDispatcher, so toggling its isEnabled state has no effect. An OnBackPressedCallback only intercepts back presses after it is added to a dispatcher. This orphaned callback creates dead code and makes the back-handling logic harder to follow.
🤖 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/ActionModeBinding.kt`
around lines 45 - 50, The OnBackPressedCallback instance
actionModeCancelCallback is never added to the back-press dispatcher so toggling
its isEnabled is ineffective; either remove the unused actionModeCancelCallback
and its isEnabled toggles, or register it with the dispatcher. To fix by
registering, call context.onBackPressedDispatcher.addCallback(lifecycleOwner,
actionModeCancelCallback) (or use the appropriate activity/fragment
onBackPressedDispatcher) when the binding is created so the callback will be
invoked, and keep using isEnabled to toggle it; if you choose removal, delete
actionModeCancelCallback and any code that sets its isEnabled (e.g., where you
enable/disable it) and ensure baseModel.actionMode.close(true) is invoked via
another registered handler.
Comment on lines
+263
to
+287
| fun moveSelectedNotesUp() { | ||
| val lastSelectedId = | ||
| notesAdapter?.getItem(lastSelectedNotePosition)?.let { (it as? BaseNote)?.id } | ||
| notesAdapter?.moveSelectedNotesUp(lastSelectedId)?.let { (topPos, lastSelectedPos) -> | ||
| binding?.MainListView?.scrollToPosition(topPos) | ||
| if (lastSelectedPos != -1) { | ||
| lastSelectedNotePosition = lastSelectedPos | ||
| } | ||
| updateActionModeSelectedNotes() | ||
| updateDatabaseSortIndices() | ||
| } | ||
| } | ||
|
|
||
| fun moveSelectedNotesDown() { | ||
| val lastSelectedId = | ||
| notesAdapter?.getItem(lastSelectedNotePosition)?.let { (it as? BaseNote)?.id } | ||
| notesAdapter?.moveSelectedNotesDown(lastSelectedId)?.let { (topPos, lastSelectedPos) -> | ||
| binding?.MainListView?.scrollToPosition(topPos) | ||
| if (lastSelectedPos != -1) { | ||
| lastSelectedNotePosition = lastSelectedPos | ||
| } | ||
| updateActionModeSelectedNotes() | ||
| updateDatabaseSortIndices() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Don't persist manual order from a filtered list.
These paths save whatever notesAdapter.currentList currently contains. On search/label/unlabeled screens that is only a visible subset, so drag-and-drop or the move up/down actions will rewrite sortIdx for some notes and leave the hidden ones stale. The next full reload can then produce duplicate or ambiguous manual ordering. Restrict manual reordering to full folder lists, or build the update map from the unfiltered dataset before calling updateNotesSortIndices().
Also applies to: 298-301, 370-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/NotallyFragment.kt`
around lines 263 - 287, The moveSelectedNotesUp/moveSelectedNotesDown handlers
(and any code that calls updateDatabaseSortIndices/updateNotesSortIndices) are
persisting order using notesAdapter.currentList which may be filtered; instead
either skip persisting when the list is filtered (only allow manual reorder on
full folder view) or build the new sort-index map against the unfiltered/full
dataset before calling updateNotesSortIndices/updateDatabaseSortIndices.
Concretely: detect a filtered state (e.g., compare notesAdapter.currentList size
vs the full notes list or use a filter flag) inside
moveSelectedNotesUp/moveSelectedNotesDown and do not call the DB update if
filtered, or construct an ID->newIndex map by iterating the full notes
collection and applying the new positions from the visible list while preserving
relative positions of hidden items, then pass that map into
updateNotesSortIndices/updateDatabaseSortIndices so hidden notes’ sortIdx remain
consistent.
Comment on lines
+162
to
+168
| layout.NotesSortDirectionRadioGroup.isVisible = | ||
| (layout.NotesSortByRadioGroup.checkedTag() as? NotesSortBy)?.isManualSort?.not() ?: true | ||
| layout.NotesSortByRadioGroup.setOnCheckedChangeListener { group, checkedId -> | ||
| val selectedSortBy = group.checkedTag() as? NotesSortBy | ||
| layout.NotesSortDirectionRadioGroup.isVisible = | ||
| selectedSortBy?.isManualSort?.not() ?: true | ||
| } |
Contributor
There was a problem hiding this comment.
Direction picker visibility logic is correct, but label is not hidden.
The logic to hide NotesSortDirectionRadioGroup when manual sort is selected is correct. However, NotesSortDirectionRadioGroupLabel should also be hidden for a consistent UI (see related comment on dialog_notes_sort.xml).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`
around lines 162 - 168, The visibility toggle only updates
NotesSortDirectionRadioGroup but not its label; update the initial visibility
and the listener to also set layout.NotesSortDirectionRadioGroupLabel.isVisible
to the same boolean you compute (use the same expression as for
NotesSortDirectionRadioGroup), i.e. read selected sort via
layout.NotesSortByRadioGroup.checkedTag() as? NotesSortBy and set both
layout.NotesSortDirectionRadioGroup.isVisible and
layout.NotesSortDirectionRadioGroupLabel.isVisible accordingly in the initial
assignment and inside the setOnCheckedChangeListener callback so the label
hides/shows in sync with the direction radio group.
Comment on lines
+613
to
+618
| private fun currentFragment(): NotallyFragment? = | ||
| supportFragmentManager | ||
| .findFragmentById(R.id.NavHostFragment) | ||
| ?.childFragmentManager | ||
| ?.fragments | ||
| ?.firstOrNull() as? NotallyFragment |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Android Navigation/Fragment APIs, is childFragmentManager.fragments.firstOrNull()a reliable way to get the currently displayed destination fragment, or shouldprimaryNavigationFragment be used instead?
💡 Result:
childFragmentManager.fragments.firstOrNull() is not a reliable way to identify the currently displayed Navigation destination fragment. That list is just “all fragments currently added” (and can temporarily include multiple fragments during transitions/animations, or keep fragments around in more complex setups), so “first” can easily be the wrong one.
For Navigation + fragments, the reliable API is primaryNavigationFragment:
- The Navigation
FragmentNavigatoris designed to callFragmentTransaction.setPrimaryNavigationFragment()for each navigation so the current destination fragment can be retrieved viaFragmentManager.getPrimaryNavigationFragment()/primaryNavigationFragment. [1] NavHostFragment/Navigation docs likewise rely on the “primary navigation fragment” concept (e.g.,setPrimaryNavigationFragment(...)is what makes a host act like the default nav host). [2]- Navigation release notes explicitly note the behavior that the current Fragment is set as the primary navigation fragment. [3]
Typical pattern:
val navHost = supportFragmentManager.findFragmentById(R.id.nav_host_fragment) as NavHostFragment
val currentDestinationFragment = navHost.childFragmentManager.primaryNavigationFragmentSources: [1] [2] [3]
Resolve the current destination via primaryNavigationFragment.
childFragmentManager.fragments.firstOrNull() returns all fragments currently added to the manager, not the currently displayed destination. After navigation or back-stack changes, this can return a stale NotallyFragment, causing prepareNewNoteIntent() and manual-sort actions to target the wrong screen. The Navigation framework uses primaryNavigationFragment as the reliable API for the current destination.
Suggested fix
private fun currentFragment(): NotallyFragment? =
- supportFragmentManager
- .findFragmentById(R.id.NavHostFragment)
- ?.childFragmentManager
- ?.fragments
- ?.firstOrNull() as? NotallyFragment
+ (supportFragmentManager.findFragmentById(R.id.NavHostFragment) as? NavHostFragment)
+ ?.childFragmentManager
+ ?.primaryNavigationFragment as? NotallyFragment📝 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
| private fun currentFragment(): NotallyFragment? = | |
| supportFragmentManager | |
| .findFragmentById(R.id.NavHostFragment) | |
| ?.childFragmentManager | |
| ?.fragments | |
| ?.firstOrNull() as? NotallyFragment | |
| private fun currentFragment(): NotallyFragment? = | |
| (supportFragmentManager.findFragmentById(R.id.NavHostFragment) as? NavHostFragment) | |
| ?.childFragmentManager | |
| ?.primaryNavigationFragment as? NotallyFragment |
🤖 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/MainActivity.kt`
around lines 613 - 618, The currentFragment() helper incorrectly uses
childFragmentManager.fragments.firstOrNull(), which can return stale fragments;
update it to resolve the displayed destination via the NavHost's
primaryNavigationFragment instead: locate the call in currentFragment() (and any
code paths like prepareNewNoteIntent() or manual-sort actions that rely on it),
replace the childFragmentManager.fragments.firstOrNull() lookup with
childFragmentManager.primaryNavigationFragment, and cast that result to
NotallyFragment (as? NotallyFragment) so the app always targets the actual
current fragment shown by the Navigation component.
Comment on lines
+151
to
+166
| when (service.status.value) { | ||
| Status.READY -> { | ||
| Stop.isEnabled = false | ||
| Main.setText(R.string.start) | ||
| } | ||
| Status.RECORDING -> { | ||
| Timer.start() | ||
| Stop.isEnabled = true | ||
| Main.setText(R.string.pause) | ||
| } | ||
| Status.PAUSED -> { | ||
| Timer.stop() | ||
| Stop.isEnabled = true | ||
| Main.setText(R.string.resume) | ||
| } | ||
| else -> {} |
Contributor
There was a problem hiding this comment.
Reset the chronometer in READY and fallback states.
Status.READY only updates the buttons, so a prior RECORDING state can leave the chronometer running. The empty else has the same stale-UI risk for null or future values.
Proposed fix
binding.apply {
Timer.base = service.getBase()
when (service.status.value) {
Status.READY -> {
+ Timer.stop()
Stop.isEnabled = false
Main.setText(R.string.start)
}
Status.RECORDING -> {
Timer.start()
Stop.isEnabled = true
Main.setText(R.string.pause)
}
Status.PAUSED -> {
Timer.stop()
Stop.isEnabled = true
Main.setText(R.string.resume)
}
- else -> {}
+ else -> {
+ Timer.stop()
+ Stop.isEnabled = false
+ Main.setText(R.string.start)
+ }
}
}📝 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
| when (service.status.value) { | |
| Status.READY -> { | |
| Stop.isEnabled = false | |
| Main.setText(R.string.start) | |
| } | |
| Status.RECORDING -> { | |
| Timer.start() | |
| Stop.isEnabled = true | |
| Main.setText(R.string.pause) | |
| } | |
| Status.PAUSED -> { | |
| Timer.stop() | |
| Stop.isEnabled = true | |
| Main.setText(R.string.resume) | |
| } | |
| else -> {} | |
| when (service.status.value) { | |
| Status.READY -> { | |
| Timer.stop() | |
| Stop.isEnabled = false | |
| Main.setText(R.string.start) | |
| } | |
| Status.RECORDING -> { | |
| Timer.start() | |
| Stop.isEnabled = true | |
| Main.setText(R.string.pause) | |
| } | |
| Status.PAUSED -> { | |
| Timer.stop() | |
| Stop.isEnabled = true | |
| Main.setText(R.string.resume) | |
| } | |
| else -> { | |
| Timer.stop() | |
| Stop.isEnabled = false | |
| Main.setText(R.string.start) | |
| } | |
| } |
🤖 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/RecordAudioActivity.kt`
around lines 151 - 166, The READY branch currently only updates buttons and can
leave Timer running from a prior RECORDING state; update the Status.READY
handling to explicitly stop and reset the chronometer (use the same Timer
control used elsewhere, e.g., call Timer.stop() and reset its base/elapsed
state) in addition to setting Stop.isEnabled = false and
Main.setText(R.string.start), and apply the same stop+reset logic in the empty
else fallback so null or unexpected service.status.value also clears the UI
timer.
Comment on lines
401
to
403
| fun RadioGroup.checkedTag(): Any { | ||
| return this.findViewById<RadioButton?>(this.checkedRadioButtonId).tag | ||
| return this.findViewById<RadioButton>(this.checkedRadioButtonId).tag | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of checkedTag() to assess crash risk
rg -n -B3 -A3 'checkedTag\(\)' --type=ktRepository: Crustack/NotallyX
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for all usages of checkedTag() without file type filter
rg -n -B3 -A3 'checkedTag\(\)' --type kotlinRepository: Crustack/NotallyX
Length of output: 9062
🏁 Script executed:
#!/bin/bash
# If kotlin type doesn't work, try searching without type filter
rg -n -B3 -A3 'checkedTag\(\)'Repository: Crustack/NotallyX
Length of output: 9062
🏁 Script executed:
#!/bin/bash
# Also search for the PreferenceBindingExtensions.kt file and read the relevant lines
find . -name "PreferenceBindingExtensions.kt" -type fRepository: Crustack/NotallyX
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Read PreferenceBindingExtensions.kt around the unsafe cast usages to see dialog initialization
sed -n '175,195p' app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.ktRepository: Crustack/NotallyX
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Read MainActivity.kt around line 700 to see dialog setup
sed -n '690,715p' app/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktRepository: Crustack/NotallyX
Length of output: 1341
🏁 Script executed:
#!/bin/bash
# Check what initializes these radio groups - search for checkedRadioButtonId or check() calls
rg -n "check\(|checkedRadioButtonId|NotesSortByRadioGroup" --context=2 | head -100Repository: Crustack/NotallyX
Length of output: 11744
🌐 Web query:
Android RadioButton tag property nullable
💡 Result:
Yes—RadioButton’s tag is nullable.
RadioButtoninheritsandroid.view.View, whosegetTag()returns the object stored as a tag ornullif not set. [1]- In XML,
android:tagis optional; if you don’t set it,getTag()will returnnull. The attribute is described as supplying a tag retrievable later viaView.getTag()/findViewWithTag(). [2]
So in Kotlin you should treat it as Any? (nullable), e.g. val t = radioButton.tag as? String.
Sources
[1] Android Developers API Reference — android.view.View (getTag() “or null if not set”) (developer.android.com)
[2] Android Developers API Reference — android.view.View (android:tag attribute description) (developer.android.com)
Citations:
- 1: https://developer.android.com/reference/android/view/View?utm_source=openai
- 2: https://developer.android.com/reference/android/view/View.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check if RadioButton extends View and what tag property is
rg -n "\.tag\s*=" | head -20Repository: Crustack/NotallyX
Length of output: 219
Fix NPE risk in RadioGroup.checkedTag() — multiple unsafe cast sites will crash.
When checkedRadioButtonId returns -1 (no selection), findViewById(-1) returns null, causing an NPE on .tag access. Additionally, RadioButton.tag is nullable, so accessing it directly is unsafe. Found 6 unsafe cast sites using as without null handling (MainActivity.kt lines 700, 705; PreferenceBindingExtensions.kt lines 188, 191, 243, 289) that will crash if the radiogroup has no selection or the tag is null.
🛡️ Proposed fix to handle missing selection
-fun RadioGroup.checkedTag(): Any {
- return this.findViewById<RadioButton>(this.checkedRadioButtonId).tag
+fun RadioGroup.checkedTag(): Any? {
+ val checkedId = this.checkedRadioButtonId
+ if (checkedId == -1) return null
+ return this.findViewById<RadioButton>(checkedId)?.tag
}Alternatively, if radio groups are guaranteed to always have a selection in this app's UI, add a defensive check with a clear exception:
-fun RadioGroup.checkedTag(): Any {
- return this.findViewById<RadioButton>(this.checkedRadioButtonId).tag
+fun RadioGroup.checkedTag(): Any {
+ val checkedId = this.checkedRadioButtonId
+ require(checkedId != -1) { "RadioGroup has no checked RadioButton" }
+ return this.findViewById<RadioButton>(checkedId).tag
}📝 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 RadioGroup.checkedTag(): Any { | |
| return this.findViewById<RadioButton?>(this.checkedRadioButtonId).tag | |
| return this.findViewById<RadioButton>(this.checkedRadioButtonId).tag | |
| } | |
| fun RadioGroup.checkedTag(): Any? { | |
| val checkedId = this.checkedRadioButtonId | |
| if (checkedId == -1) return null | |
| return this.findViewById<RadioButton>(checkedId)?.tag | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt` around
lines 401 - 403, RadioGroup.checkedTag() currently dereferences
findViewById(...) and RadioButton.tag unsafely which causes NPE when
checkedRadioButtonId == -1 or tag is null; change the function to safely handle
those cases by returning a nullable type (e.g., Any?) or throwing a clear
IllegalStateException with context, and update all caller sites using unsafe
"as" casts to handle null or perform safe casting; specifically modify
RadioGroup.checkedTag() to check checkedRadioButtonId != -1, null-check the
result of findViewById<RadioButton>(...), and return the nullable tag (or throw)
so callers in MainActivity and PreferenceBindingExtensions replace "as" with
safe casts or null-handling.
Comment on lines
175
to
177
| fun setSearchKeyword(keyword: String) { | ||
| Log.d("SearchResult", "keyword: $keyword") | ||
| if (searchKeyword != keyword) { |
Contributor
There was a problem hiding this comment.
Remove the keyword debug log.
Search text can contain note contents, passwords, or other PII, and this prints it verbatim on every keystroke.
🤖 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/view/main/BaseNoteAdapter.kt`
around lines 175 - 177, Remove the verbose debug log that prints user search
input in setSearchKeyword; locate the Log.d("SearchResult", "keyword: $keyword")
call inside the setSearchKeyword function and delete it (or replace it with a
non-PII-safe log such as a flag/length check or obfuscated/hashed value),
ensuring only non-sensitive information or no logging of the raw keyword remains
while leaving the rest of the setSearchKeyword logic intact.
Comment on lines
+272
to
+345
| fun moveSelectedNotesUp(lastSelectedId: Long?): Pair<Int, Int>? { | ||
| if (list is MutableListContainer) { | ||
| val items = list.toList() | ||
| val selectedIndices = | ||
| items.mapIndexedNotNull { index, item -> | ||
| if (item is BaseNote && selectedIds.contains(item.id)) index else null | ||
| } | ||
| if (selectedIndices.isEmpty()) return null | ||
|
|
||
| var moved = false | ||
| selectedIndices.forEachIndexed { i, index -> | ||
| if (index > i) { | ||
| (list as MutableListContainer).move(index, index - 1) | ||
| moved = true | ||
| } | ||
| } | ||
| if (!moved) return null | ||
|
|
||
| val newItems = list.toList() | ||
| val topMostMovedPosition = | ||
| newItems | ||
| .indexOfFirst { it is BaseNote && selectedIds.contains(it.id) } | ||
| .takeIf { it != -1 } | ||
|
|
||
| val newLastSelectedPos = | ||
| if (lastSelectedId != null) { | ||
| newItems.indexOfFirst { it is BaseNote && it.id == lastSelectedId } | ||
| } else -1 | ||
|
|
||
| updateSortIndices() | ||
| return topMostMovedPosition?.let { it to newLastSelectedPos } | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| /** | ||
| * @return a pair of (top-most moved position, new position of the last selected note). If there | ||
| * is no moved position, return null. | ||
| */ | ||
| fun moveSelectedNotesDown(lastSelectedId: Long?): Pair<Int, Int>? { | ||
| if (list is MutableListContainer) { | ||
| val items = list.toList() | ||
| val selectedIndices = | ||
| items.mapIndexedNotNull { index, item -> | ||
| if (item is BaseNote && selectedIds.contains(item.id)) index else null | ||
| } | ||
| if (selectedIndices.isEmpty()) return null | ||
|
|
||
| var moved = false | ||
| val n = items.size | ||
| selectedIndices.reversed().forEachIndexed { i, index -> | ||
| if (index < n - 1 - i) { | ||
| (list as MutableListContainer).move(index, index + 1) | ||
| moved = true | ||
| } | ||
| } | ||
| if (!moved) return null | ||
|
|
||
| val newItems = list.toList() | ||
| val topMostMovedPosition = | ||
| newItems | ||
| .indexOfFirst { it is BaseNote && selectedIds.contains(it.id) } | ||
| .takeIf { it != -1 } | ||
|
|
||
| val newLastSelectedPos = | ||
| if (lastSelectedId != null) { | ||
| newItems.indexOfFirst { it is BaseNote && it.id == lastSelectedId } | ||
| } else -1 | ||
|
|
||
| updateSortIndices() | ||
| return topMostMovedPosition?.let { it to newLastSelectedPos } | ||
| } | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the move-up/down actions inside the current section.
These helpers only check absolute list bounds. A selected note at the edge of a section can move across a Header or into the other pinned state, even though drag-and-drop explicitly blocks that path. Apply the same adjacent-item and pinned-state guard here before calling move().
🤖 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/view/main/BaseNoteAdapter.kt`
around lines 272 - 345, In moveSelectedNotesUp and moveSelectedNotesDown,
prevent moving a selected BaseNote across section boundaries by checking the
adjacent item and pinned-state before calling (list as
MutableListContainer).move: for each candidate index, fetch the neighboring item
(index-1 for up, index+1 for down) and skip the move if that neighbor is a
Header or if its pinned state differs from the selected item's pinned state; use
the original items list to inspect neighbors and the BaseNote.pinned (or
equivalent) flag and only call move when both adjacent-item and pinned-state
guards pass, preserving all other logic (selectedIndices iteration, moved flag,
computing new positions, updateSortIndices).
Comment on lines
+660
to
+672
| fun updateNoteSortIdx(id: Long, sortIdx: Int) { | ||
| viewModelScope.launch { | ||
| withContext(Dispatchers.IO) { baseNoteDao.updateSortIdx(id, sortIdx) } | ||
| } | ||
| } | ||
|
|
||
| fun updateNotesSortIndices(idToIdx: Map<Long, Int>) { | ||
| viewModelScope.launch { | ||
| withContext(Dispatchers.IO) { baseNoteDao.updateSortIndices(idToIdx) } | ||
| } | ||
| } | ||
|
|
||
| suspend fun getMaxNoteSortIdx() = withContext(Dispatchers.IO) { baseNoteDao.getMaxSortIdx() } |
Contributor
There was a problem hiding this comment.
Duplicates still reuse the source sortIdx.
These new sort-index helpers do not cover the duplication path in this same ViewModel: duplicateNotes() at Lines 742-758 does deepCopy().copy(...) and then calls baseNoteDao.insert(copies), so the copied note keeps the original manual position. That creates sortIdx collisions and makes manual ordering ambiguous. Reset sortIdx on duplicates and insert them through insertSafe(app, copies) instead.
Proposed fix
val copies: List<BaseNote> =
notes.map { original ->
original
.deepCopy()
.copy(
id = 0L,
title =
if (original.title.isNotEmpty())
"${original.title} (${app.getString(R.string.copy)})"
else app.getString(R.string.copy),
timestamp = now,
modifiedTimestamp = now,
+ sortIdx = null,
)
}
- return withContext(Dispatchers.IO) { baseNoteDao.insert(copies) }
+ return withContext(Dispatchers.IO) { baseNoteDao.insertSafe(app, copies) }🤖 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 - 672, The duplicateNotes() flow currently deep-copies notes
and calls baseNoteDao.insert(copies) causing duplicates to inherit the original
sortIdx; update duplicateNotes() so each copied note resets its sortIdx (e.g.,
set sortIdx = null or a sentinel so ordering logic will assign a new index) when
calling deepCopy().copy(...), and replace the direct baseNoteDao.insert(copies)
call with insertSafe(app, copies) so inserts go through the safe insertion path
that assigns proper sort indices; reference duplicateNotes(),
deepCopy().copy(...), baseNoteDao.insert, and insertSafe(app, copies) when
making this change.
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 #130
notallyx_issues_130_rc1.webm
Summary by CodeRabbit
New Features
Documentation