Conversation
Contributor
📝 WalkthroughWalkthroughAdds persistent, user-reorderable label ordering: DB schema v11 with an Changes
Sequence DiagramsequenceDiagram
actor User
participant LabelsFragment
participant ItemTouchHelper as ItemTouchHelper\n(SimpleCallback)
participant LabelAdapter
participant ViewModel as BaseNoteModel
participant LabelDao
participant DB as RoomDB
User->>LabelsFragment: Long-press drag handle
LabelsFragment->>ItemTouchHelper: startDrag(viewHolder)
User->>ItemTouchHelper: Drag to new position
ItemTouchHelper->>LabelAdapter: onItemMove(fromPos, toPos)
LabelAdapter->>LabelAdapter: Reorder in-memory list
User->>ItemTouchHelper: Release
ItemTouchHelper->>LabelsFragment: clearView()
LabelsFragment->>ViewModel: updateLabels(List<Label>)
ViewModel->>LabelDao: update(labels)
LabelDao->>DB: UPDATE Label SET order=?
DB-->>LabelDao: OK
LabelDao-->>ViewModel: Complete
ViewModel-->>LabelsFragment: LiveData updates UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
app/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.kt (1)
58-66: Unify label ordering strategy across import paths.This path sorts before assigning order, while
app/src/main/java/com/philkes/notallyx/data/dao/CommonDao.kt(Line 49-74) assigns order without sorting. Same input labels can therefore end up in different final orders depending on import flow.♻️ Suggested direction
- labelDao.insert( - notes - .flatMap { it.labels } - .distinct() - .sorted() - .mapIndexed { index, value -> Label(value, maxOrder + 1 + index) } - ) + // Consider delegating to a shared helper used by both NotesImporter and CommonDao + // so ordering rules are identical for all import/restore flows. + labelDao.insert( + notes + .flatMap { it.labels } + .distinct() + .sorted() // or preserve source order — but keep it consistent everywhere + .mapIndexed { index, value -> Label(value, maxOrder + 1 + index) } + )🤖 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/imports/NotesImporter.kt` around lines 58 - 66, The import path in NotesImporter.kt currently sorts labels before assigning order (labels.flatMap { it.labels }.distinct().sorted().mapIndexed { ... }), which conflicts with the ordering strategy used in CommonDao.kt where order is assigned without sorting; make the two paths consistent by removing the .sorted() call so labels.flatMap { it.labels }.distinct().mapIndexed { index, value -> Label(value, maxOrder + 1 + index) } is used when calling labelDao.insert; keep use of labelDao.getMaxOrder(), labelDao.insert(...) and the Label(value, maxOrder + 1 + index) construction unchanged.app/src/main/java/com/philkes/notallyx/data/dao/CommonDao.kt (1)
68-72: Consider extracting shared label-order remap logic.Both
importBackupoverloads repeat the samegetMaxOrder()+mapIndexedinsertion block. A private helper would keep this behavior centralized and safer to evolve.Also applies to: 141-145
🤖 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/CommonDao.kt` around lines 68 - 72, Extract the repeated getMaxOrder() + mapIndexed insertion logic into a private helper (e.g., a method like insertLabelsWithRemappedOrder(labelDao: LabelDao, labels: List<LabelDto>) or similar) and call it from both importBackup overloads; the helper should call labelDao.getMaxOrder() ?: -1, compute new Labels with mapIndexed using maxOrder + 1 + index, and perform labelDao.insert(...) so the remapping behavior is centralized (replace the duplicated block around labelDao, getMaxOrder, and mapIndexed at the importBackup sites).
🤖 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/NotallyDatabase.kt`:
- Around line 319-328: The migration backfill iterates labels with "SELECT value
FROM Label ORDER BY value DESC" and assigns order = 0..n, which reverses the
original ordering; fix it by preserving original order—either query with ORDER
BY value ASC (so incremental order matches existing display order) or compute
the total count first and assign order starting from count-1 down to 0 while
iterating DESC; update the migration block that uses the cursor and db.execSQL
on the Label table (the code that sets the `order` column for each value) so the
final `order` values preserve the legacy display order.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt`:
- Around line 69-71: clearView() is persisting a stale order because
LabelAdapter.onItemMove() calls submitList() (async via DiffUtil) and
clearView() then reads labelAdapter?.currentList; change the flow so clearView()
uses the reordered list produced by onItemMove() instead of re-reading
currentList (or wait for submitList completion). Concretely, modify
LabelAdapter.onItemMove(fromPosition: Int, toPosition: Int) to return the new
reordered list (List<Label>) (or provide a callback) and in the drag handling
code capture that return value when calling
labelAdapter?.onItemMove(fromPosition, toPosition) and pass that list to the
ViewModel/database persist logic in clearView(), removing the direct use of
labelAdapter?.currentList so the DB write reflects the actual reordered state.
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelAdapter.kt`:
- Around line 23-27: In onItemMove, guard the indices before mutating the
adapter list: validate that fromPosition and toPosition are within
0..currentList.size-1 (and short-circuit/return currentList unchanged if not)
before calling removeAt(fromPosition) and add(toPosition, ...); only call
submitList(list) and return the new list when the indices are valid to avoid
crashes from transient invalid adapter positions.
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelVH.kt`:
- Around line 15-22: The long-click handlers for LabelText and DragHandle are
returning false which doesn't consume the event and allows the click listener to
also fire; change both lambdas to return true after calling
listener.onStartDrag(this@LabelVH) so the long-press is consumed and the click
handler won't run during drag initiation (update the setOnLongClickListener for
LabelText and DragHandle in LabelVH accordingly).
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.kt`:
- Around line 27-35: LabelDiffCallback currently uses reference equality in
areItemsTheSame and only compares value in areContentsTheSame; because Label is
a plain Room entity (new instances per emission) you should compare a stable key
(e.g., id) in areItemsTheSame and include mutable fields (e.g., value and order)
in areContentsTheSame. Update LabelDiffCallback.areItemsTheSame to return
oldItem.id == newItem.id and update areContentsTheSame to compare all relevant
properties (value and order) so DiffUtil can correctly detect moves/updates.
In
`@app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt`:
- Around line 212-217: The code assigns new label orders with maxOrder + 1 +
index which, because labels are read ORDER BY order DESC, reverses
restored/imported lists; change the assignment so the first item in
previousLabels gets the highest order (e.g. compute order as maxOrder +
previousLabels.size - index) in the mapping used with labelDao.insert
(reference: labelDao.getMaxOrder(), previousLabels.mapIndexed, Label
constructor) and apply the same fix to the analogous logic in NotesImporter.kt
to preserve input display order.
- Around line 779-783: Change LabelDao.insert to return Long so callers can
detect ignored inserts and update insertLabel to check that return value: modify
the DAO method signature for insert (LabelDao.insert) to return Long (so it
returns -1L on conflict), then in BaseNoteModel.insertLabel use the returned
Long from labelDao.insert inside the executeAsyncWithCallback task and treat any
result <= 0 (e.g., -1L) as a failure (call onComplete(false)) and only call
onComplete(true) when the insert returns a positive rowId; keep using
executeAsyncWithCallback and SQLite exception handling for other errors.
In `@app/src/main/res/layout/recycler_label.xml`:
- Around line 8-16: The DragHandle ImageView uses
android:contentDescription="@null" and may have too-small touch target; for
accessibility update the ImageView with a meaningful contentDescription (e.g.,
reference a new string resource like `@string/drag_handle_description`), mark it
focusable/clickable if it starts drags (android:focusable="true"
android:clickable="true"), and increase its touch target to at least 48dp (use
android:minWidth="48dp" and android:minHeight="48dp" or wrap it in a larger
touchable container) so TalkBack users can discover and activate the drag
handle; keep the id DragHandle to locate the element.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/data/dao/CommonDao.kt`:
- Around line 68-72: Extract the repeated getMaxOrder() + mapIndexed insertion
logic into a private helper (e.g., a method like
insertLabelsWithRemappedOrder(labelDao: LabelDao, labels: List<LabelDto>) or
similar) and call it from both importBackup overloads; the helper should call
labelDao.getMaxOrder() ?: -1, compute new Labels with mapIndexed using maxOrder
+ 1 + index, and perform labelDao.insert(...) so the remapping behavior is
centralized (replace the duplicated block around labelDao, getMaxOrder, and
mapIndexed at the importBackup sites).
In `@app/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.kt`:
- Around line 58-66: The import path in NotesImporter.kt currently sorts labels
before assigning order (labels.flatMap { it.labels
}.distinct().sorted().mapIndexed { ... }), which conflicts with the ordering
strategy used in CommonDao.kt where order is assigned without sorting; make the
two paths consistent by removing the .sorted() call so labels.flatMap {
it.labels }.distinct().mapIndexed { index, value -> Label(value, maxOrder + 1 +
index) } is used when calling labelDao.insert; keep use of
labelDao.getMaxOrder(), labelDao.insert(...) and the Label(value, maxOrder + 1 +
index) construction unchanged.
🪄 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: 73414572-2ca6-4154-9ad3-d3e5a3b77a42
📒 Files selected for processing (22)
app/schemas/com.philkes.notallyx.data.NotallyDatabase/11.jsonapp/src/main/java/com/philkes/notallyx/data/NotallyDatabase.ktapp/src/main/java/com/philkes/notallyx/data/dao/CommonDao.ktapp/src/main/java/com/philkes/notallyx/data/dao/LabelDao.ktapp/src/main/java/com/philkes/notallyx/data/imports/NotesImporter.ktapp/src/main/java/com/philkes/notallyx/data/model/Label.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/MainActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/SelectLabelsActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelListener.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelVH.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelVH.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/utils/backup/ImportExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/MigrationsExtensions.ktapp/src/main/java/com/philkes/notallyx/utils/backup/XmlParserExtensions.ktapp/src/main/res/drawable/notification_unpin.xmlapp/src/main/res/layout-v26/recycler_label.xmlapp/src/main/res/layout/recycler_label.xml
Comment on lines
+27
to
+35
| class LabelDiffCallback() : DiffUtil.ItemCallback<Label>() { | ||
|
|
||
| override fun areItemsTheSame(oldItem: Label, newItem: Label): Boolean { | ||
| return oldItem == newItem | ||
| } | ||
|
|
||
| override fun areContentsTheSame(oldItem: Label, newItem: Label): Boolean { | ||
| return oldItem.value == newItem.value | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Label equality semantics and the current DiffUtil implementation.
rg -n -C2 'data class Label|class Label' app/src/main/java/com/philkes/notallyx/data/model/Label.kt
rg -n -C2 'areItemsTheSame|areContentsTheSame|oldItem == newItem|oldItem.value == newItem.value' app/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.ktRepository: Crustack/NotallyX
Length of output: 508
Use stable key comparison in LabelDiffCallback.
Line 30 compares Label with ==, but Label is a plain class, so this is reference equality. Every fresh Room emission creates a new object instance and will look like a different item. Additionally, Line 34 ignores the mutable order field when checking content equality.
Suggested fix
class LabelDiffCallback() : DiffUtil.ItemCallback<Label>() {
override fun areItemsTheSame(oldItem: Label, newItem: Label): Boolean {
- return oldItem == newItem
+ return oldItem.value == newItem.value
}
override fun areContentsTheSame(oldItem: Label, newItem: Label): Boolean {
- return oldItem.value == newItem.value
+ return oldItem.order == newItem.order
}
}🤖 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/label/SelectableLabelAdapter.kt`
around lines 27 - 35, LabelDiffCallback currently uses reference equality in
areItemsTheSame and only compares value in areContentsTheSame; because Label is
a plain Room entity (new instances per emission) you should compare a stable key
(e.g., id) in areItemsTheSame and include mutable fields (e.g., value and order)
in areContentsTheSame. Update LabelDiffCallback.areItemsTheSame to return
oldItem.id == newItem.id and update areContentsTheSame to compare all relevant
properties (value and order) so DiffUtil can correctly detect moves/updates.
Comment on lines
+8
to
+16
| <ImageView | ||
| android:id="@+id/DragHandle" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="center_vertical" | ||
| android:padding="8dp" | ||
| android:src="@drawable/drag_handle" | ||
| app:tint="?attr/colorControlNormal" | ||
| android:contentDescription="@null" /> |
Contributor
There was a problem hiding this comment.
Make the drag handle accessible for assistive tech users.
If this view is interactive (drag start), android:contentDescription="@null" leaves it unlabeled for TalkBack and the hit area is likely below recommended size.
♿ Suggested XML adjustment
<ImageView
android:id="@+id/DragHandle"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center_vertical"
+ android:minWidth="48dp"
+ android:minHeight="48dp"
android:padding="8dp"
android:src="@drawable/drag_handle"
app:tint="?attr/colorControlNormal"
- android:contentDescription="@null" />
+ android:contentDescription="@string/reorder_label" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/res/layout/recycler_label.xml` around lines 8 - 16, The
DragHandle ImageView uses android:contentDescription="@null" and may have
too-small touch target; for accessibility update the ImageView with a meaningful
contentDescription (e.g., reference a new string resource like
`@string/drag_handle_description`), mark it focusable/clickable if it starts drags
(android:focusable="true" android:clickable="true"), and increase its touch
target to at least 48dp (use android:minWidth="48dp" and
android:minHeight="48dp" or wrap it in a larger touchable container) so TalkBack
users can discover and activate the drag handle; keep the id DragHandle to
locate the element.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.kt (1)
29-31:⚠️ Potential issue | 🟠 MajorUse stable key identity in
areItemsTheSame(not object reference).
oldItem == newItemis reference-based here, so DiffUtil won’t reliably match the same label across Room emissions. Compare the stable key (value) instead.#!/bin/bash # Verify Label model equality semantics and DiffUtil identity comparison. rg -n -C2 '@Entity class Label|data class Label|class Label' app/src/main/java/com/philkes/notallyx/data/model/Label.kt rg -n -C4 'class LabelDiffCallback|areItemsTheSame|areContentsTheSame|return oldItem == newItem' app/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.ktSuggested fix
class LabelDiffCallback() : DiffUtil.ItemCallback<Label>() { override fun areItemsTheSame(oldItem: Label, newItem: Label): Boolean { - return oldItem == newItem + return oldItem.value == newItem.value } override fun areContentsTheSame(oldItem: Label, newItem: Label): Boolean { return oldItem.value == newItem.value && oldItem.order == newItem.order } }🤖 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/label/SelectableLabelAdapter.kt` around lines 29 - 31, areItemsTheSame in SelectableLabelAdapter currently uses reference equality (oldItem == newItem); change it to compare the label's stable key (e.g., return oldItem.value == newItem.value) so DiffUtil can reliably identify the same label across Room emissions. Keep areContentsTheSame unchanged (or continue using full equality) so content diffs still work; if Label has a different stable id field, use that field instead of value.
🤖 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/fragment/LabelsFragment.kt`:
- Around line 65-94: clearView() always persists currentList even when no move
happened because currentList is only updated in onMove(); fix by tracking
whether a real reorder occurred and only call model.updateLabels when positions
changed: set a boolean flag (e.g., hasMoved) to true inside onMove() when
labelAdapter?.onItemMove(fromPosition, toPosition) returns a new list or when
fromPosition != toPosition, check that flag in clearView() before mapping and
calling model.updateLabels, and reset the flag and currentList afterwards;
reference symbols: currentList, onMove(), labelAdapter?.onItemMove(...),
clearView(), and model.updateLabels(...).
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.kt`:
- Around line 29-31: areItemsTheSame in SelectableLabelAdapter currently uses
reference equality (oldItem == newItem); change it to compare the label's stable
key (e.g., return oldItem.value == newItem.value) so DiffUtil can reliably
identify the same label across Room emissions. Keep areContentsTheSame unchanged
(or continue using full equality) so content diffs still work; if Label has a
different stable id field, use that field instead of value.
🪄 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: 221ee817-1c5c-4cdb-9b49-283f68208f0a
📒 Files selected for processing (8)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelVH.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/label/SelectableLabelAdapter.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.ktapp/src/main/java/com/philkes/notallyx/utils/backup/MigrationsExtensions.ktapp/src/main/res/layout-v26/recycler_label.xmlapp/src/main/res/layout/recycler_label.xml
💤 Files with no reviewable changes (1)
- app/src/main/java/com/philkes/notallyx/utils/backup/MigrationsExtensions.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/res/layout/recycler_label.xml
- app/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelVH.kt
- app/src/main/res/layout-v26/recycler_label.xml
- app/src/main/java/com/philkes/notallyx/presentation/viewmodel/BaseNoteModel.kt
- app/src/main/java/com/philkes/notallyx/presentation/view/main/label/LabelAdapter.kt
…ain/fragment/LabelsFragment.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt (1)
80-80: Consider suppressing detekt warning for intentionally empty override.Since swipe direction is
0, this callback is never invoked. Adding a suppression annotation clarifies intent and silences the static analysis warning.♻️ Suggested fix
+ `@Suppress`("EmptyFunctionBlock") override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt` at line 80, In LabelsFragment, the empty override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) is intentional (swipe direction is 0) but triggers a detekt warning; suppress it by adding a `@Suppress`("EmptyFunctionBlock") annotation on the onSwiped override (or directly above the anonymous ItemTouchHelper.Callback implementation) to clarify intent and silence the static analysis warning while keeping the empty body.
🤖 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/fragment/LabelsFragment.kt`:
- Around line 100-102: In LabelsFragment, remove the extra closing brace that
has no matching opening brace (the duplicate '}' shown in the diff) so the
block/bracing in the LabelsFragment class (e.g., within the method where the
diff appears) is balanced; locate the stray brace inside LabelsFragment and
delete it, then recompile to confirm balanced braces and no syntax error.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt`:
- Line 80: In LabelsFragment, the empty override fun onSwiped(viewHolder:
RecyclerView.ViewHolder, direction: Int) is intentional (swipe direction is 0)
but triggers a detekt warning; suppress it by adding a
`@Suppress`("EmptyFunctionBlock") annotation on the onSwiped override (or directly
above the anonymous ItemTouchHelper.Callback implementation) to clarify intent
and silence the static analysis warning while keeping the empty body.
🪄 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: 344a2ad5-db1a-4c5b-9a67-a07e7aacac82
📒 Files selected for processing (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt (2)
154-157: Consider using safe call instead of non-null assertion.While the non-null assertions (
!!) are safe here (we're inside aletblock that already accessedlabelAdapter?.currentList), usinglabelAdapter?.let { adapter -> ... }would be more idiomatic and avoid the assertion operators.💡 Suggested improvement
override fun onToggleVisibility(position: Int) { - labelAdapter?.currentList?.get(position)?.let { value -> + labelAdapter?.let { adapter -> + adapter.currentList.getOrNull(position)?.let { value -> val hiddenLabels = model.preferences.labelsHidden.value.toMutableSet() if (value.visibleInNavigation) { hiddenLabels.add(value.value) } else { hiddenLabels.remove(value.value) } model.savePreference(model.preferences.labelsHidden, hiddenLabels) - val currentList = labelAdapter!!.currentList.toMutableList() + val currentList = adapter.currentList.toMutableList() currentList[position] = currentList[position].copy(visibleInNavigation = !value.visibleInNavigation) - labelAdapter!!.submitList(currentList) + adapter.submitList(currentList) + } } }🤖 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/LabelsFragment.kt` around lines 154 - 157, Replace the non-null assertions on labelAdapter with a safe-call + let to avoid using !!: wrap logic that reads labelAdapter.currentList, modifies the item at position (toggling value.visibleInNavigation) and calls submitList inside labelAdapter?.let { adapter -> ... } so you operate on adapter.currentList.toMutableList(), update currentList[position] and then call adapter.submitList(currentList); reference symbols: labelAdapter, currentList, position, value.visibleInNavigation, submitList.
80-80: Add explicit no-op body to satisfy static analysis.The empty
onSwipedis intentionally a no-op since swipe direction is0, but detekt flags the empty block. Consider adding a brief comment or usingUnitexplicitly.💡 Suggested fix
- override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {} + override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) { + // Swipe disabled via direction = 0 + }🤖 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/LabelsFragment.kt` at line 80, The empty override for onSwiped in LabelsFragment is flagged by detekt; make the no-op explicit by replacing the empty block with a clear no-op statement and/or comment. Locate the override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) in LabelsFragment.kt and change its body to something like { /* no-op: swipe disabled (direction == 0) */ Unit } so the function is still a no-op but not an empty block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt`:
- Around line 154-157: Replace the non-null assertions on labelAdapter with a
safe-call + let to avoid using !!: wrap logic that reads
labelAdapter.currentList, modifies the item at position (toggling
value.visibleInNavigation) and calls submitList inside labelAdapter?.let {
adapter -> ... } so you operate on adapter.currentList.toMutableList(), update
currentList[position] and then call adapter.submitList(currentList); reference
symbols: labelAdapter, currentList, position, value.visibleInNavigation,
submitList.
- Line 80: The empty override for onSwiped in LabelsFragment is flagged by
detekt; make the no-op explicit by replacing the empty block with a clear no-op
statement and/or comment. Locate the override fun onSwiped(viewHolder:
RecyclerView.ViewHolder, direction: Int) in LabelsFragment.kt and change its
body to something like { /* no-op: swipe disabled (direction == 0) */ Unit } so
the function is still a no-op but not an empty block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaab8e93-928d-434a-afd9-aef7d06a9954
📒 Files selected for processing (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/LabelsFragment.kt
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 #787
Summary by CodeRabbit
New Features
Improvements