Conversation
Contributor
📝 WalkthroughWalkthroughAdds scroll-to-top and scroll-to-bottom controls: new jump button views, orientation-aware visibility logic in EditActivity, ListManager callbacks to report item count changes, new vector icons and strings, and updated translation totals in TRANSLATIONS.md. Changes
Sequence DiagramsequenceDiagram
actor User
participant EditActivity
participant ListManager
participant BottomAppBar
participant ScrollView
User->>EditActivity: edit content / type
EditActivity->>ListManager: notify text/item change
ListManager->>EditActivity: onItemSizeChanged(size)
EditActivity->>EditActivity: updateJumpButtonsVisibility()
EditActivity->>BottomAppBar: set jump button visibility
User->>BottomAppBar: tap Jump To Top / Bottom
BottomAppBar->>ScrollView: scroll to top / bottom
ScrollView->>User: content moved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 2
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/presentation/view/note/listitem/ListManager.kt (2)
84-89:⚠️ Potential issue | 🟠 MajorUndo/redo of list-item add/delete leaves jump button visibility stale.
setStateis the entry point for every undo and redo operation on list notes. It updates the adapter anditemsCheckedbut never callsonItemSizeChanged, so after undoing a delete (item count increases) or undoing an add (item count decreases), the jump buttons won't show or hide as expected.For text notes this is already handled because
EditNoteActivity.setupListenerswiresupdateJumpButtonsVisibility()into the history callback. List notes have no equivalent hook.The simplest fix is to notify in
setState:🐛 Proposed fix
internal fun setState(state: ListState) { adapter.submitList(state.items) { state.focusedItemPos?.let { itemPos -> focusItem(itemPos, state.cursorPos) } } this.itemsChecked?.setItems(state.checkedItems!!) + onItemSizeChanged?.invoke(state.items.size + (state.checkedItems?.size ?: 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/view/note/listitem/ListManager.kt` around lines 84 - 89, setState in ListManager updates the adapter and itemsChecked but never triggers onItemSizeChanged, so undo/redo that changes item count leaves jump-button visibility stale; after adapter.submitList(state.items) completes (i.e., inside its completion callback where you already call focusItem), invoke onItemSizeChanged() to recalculate and update jump buttons (ensure you do this before or after calling focusItem as appropriate), and also ensure itemsChecked.setItems(state.checkedItems!!) is called after this update if ordering matters.
367-378:⚠️ Potential issue | 🟠 Major
items.sizeindeleteCheckedItemsreads the potentially-stale adapter property.
itemshere is the property getter (get() = adapter.items).adapter.submitList(itemsUpdated)is submitted earlier on line 372, but ifListItemAdapterusesAsyncListDiffer/DiffUtilthe underlying list reference may not have swapped by the time line 377 executes. The companion methoddelete()correctly avoids this by using its local copy;deleteCheckedItemsshould do the same.🐛 Proposed fix
adapter.submitList(itemsUpdated) itemsChecked?.deleteCheckedItems() if (pushChange) { changeHistory.push(DeleteCheckedChange(stateBefore, getState(), this)) } - onItemSizeChanged?.invoke(items.size) + onItemSizeChanged?.invoke(itemsUpdated.size) }🤖 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/note/listitem/ListManager.kt` around lines 367 - 378, The method deleteCheckedItems reads the adapter-backed property items after submitting a new list which can be asynchronously swapped; replace the stale adapter access with the local cloned list: compute the new size from itemsUpdated and call onItemSizeChanged?.invoke(itemsUpdated.size) (mirroring the pattern used in delete()), and ensure any other uses that rely on the post-submit list state use itemsUpdated instead of the adapter-backed items.
🧹 Nitpick comments (4)
app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt (1)
43-51: Misleading parameter name in callback type.
((items: Int) -> Unit)?names the lambda parameteritems, but the value is an item count (Int), not the list. This is confusing at every call site and especially at declaration.itemCountorcountwould be clearer.♻️ Proposed rename
- val onItemSizeChanged: ((items: Int) -> Unit)?, + val onItemSizeChanged: ((itemCount: Int) -> Unit)?,🤖 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/note/listitem/ListManager.kt` around lines 43 - 51, The callback parameter name onItemSizeChanged in class ListManager is misleading—rename the lambda parameter from items to itemCount (or count) in the ListManager constructor signature and update all call sites and any anonymous/lambda implementations to use the new name (onItemSizeChanged: ((itemCount: Int) -> Unit)?). Locate references by the symbol onItemSizeChanged and the ListManager constructor and update parameter documentation/comments if present.app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt (1)
644-663:getChildAt(0)null-guard is dead code.
binding.ScrollViewis the single-childNestedScrollViewwhose child layout is always inflated before the button can be pressed;getChildAt(0)will never returnnullhere. Theelsebranch is unreachable, and the explicitbinding.ScrollView.*calls insideapply { post { } }are redundant sincethisalready refers to theScrollView.♻️ Suggested simplification
-binding.ScrollView.apply { - post { - val lastChild: View? = binding.ScrollView.getChildAt(0) - if (lastChild != null) { - val bottom: Int = - lastChild.bottom + binding.ScrollView.paddingBottom - binding.ScrollView.smoothScrollTo(0, bottom) - } else { - fullScroll(View.FOCUS_DOWN) - } - } -} +binding.ScrollView.post { + val lastChild = binding.ScrollView.getChildAt(0) ?: return@post + val bottom = lastChild.bottom + binding.ScrollView.paddingBottom + binding.ScrollView.smoothScrollTo(0, bottom) +}🤖 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/EditActivity.kt` around lines 644 - 663, The null-check for getChildAt(0) in the jumpToBottom button handler is dead code and you can simplify the apply/post block: inside the addIconButton lambda (the jumpToBottom creation) remove the unreachable else branch and the explicit binding.ScrollView qualifiers, use the ScrollView instance referenced by this (from apply) and assume its single child exists (e.g., val lastChild = getChildAt(0)), compute bottom from lastChild.bottom + paddingBottom, then call smoothScrollTo(0, bottom); keep updateJumpButtonsVisibility() as-is.app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt (1)
244-250: Lambda parameteritemsshadows the outer class property.The
{ items -> ... }binding shadowsEditListActivity.items: MutableList<ListItem>. Since the parameter is actually an item count (Int), renaming it avoids confusion for future readers.♻️ Suggested rename
- { items -> updateJumpButtonsVisibility(items) }, + { count -> updateJumpButtonsVisibility(count) },🤖 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/EditListActivity.kt` around lines 244 - 250, The lambda passed to updateJumpButtonsVisibility currently names its parameter "items", which shadows the EditListActivity.items property; change the lambda parameter name (e.g., to "itemCount" or "count") so it reflects that it is an Int and does not shadow the class property, updating the usage inside the lambda accordingly (the call to updateJumpButtonsVisibility should remain unchanged).app/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt (1)
61-63: Constructor update is correct; consider capturingonItemSizeChangedto enable callback verification.Passing
nullforonItemSizeChangedis safe for the existing tests, but it means no test currently asserts that the callback is invoked (or invoked with the correct count) after item additions/deletions. Given that this callback drives the jump-button visibility in production, having at least one test that wires in a capturing lambda would protect against regressions.🔬 Suggested approach for capturing the callback in tests
+ var lastReportedItemSize: Int? = null + listManager = - ListManager(recyclerView, changeHistory, preferences, inputMethodManager, {}, {}, null) + ListManager(recyclerView, changeHistory, preferences, inputMethodManager, {}, {}) { count -> + lastReportedItemSize = count + }Individual test cases can then assert:
// e.g. after listManager.add(...) assertEquals(expectedCount, lastReportedItemSize)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt` around lines 61 - 63, The tests currently pass null for the ListManager constructor's onItemSizeChanged parameter, so no test can verify the callback; update ListManagerTestBase to provide a capturing lambda to the ListManager(...) call (reference: ListManager constructor and listManager variable) that writes the reported count into a test-scoped var (e.g. lastReportedItemSize) initialized in the test base, and then update/add tests to assert expected values after operations (e.g. after listManager.add/... or remove) to validate the callback is invoked with the correct count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt`:
- Around line 552-567: Remove the debug Log.d call in
updateJumpButtonsVisibility and avoid reading binding.EnterBody.lineCount for
LIST-type notes; compute the candidate size as manualSize ?: if
(notallyModel.type == Type.NOTE) binding.EnterBody.lineCount else
notallyModel.items.size, then use that value in the existing show calculation
and set jumpToTop/isVisible and jumpToBottom/isVisible as before, leaving
function name updateJumpButtonsVisibility, binding.EnterBody.lineCount,
notallyModel.type, Type.LIST, and notallyModel.items references to locate the
change.
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt`:
- Around line 154-155: onItemSizeChanged currently passes only unchecked item
count because items (which delegates to adapter.items) excludes checked items
when autoSortByChecked is enabled; update every onItemSizeChanged?.invoke(...)
call (e.g., the ones near usages of updateJumpButtonsVisibility and manualSize)
to pass the combined total of unchecked plus checked items by summing items.size
and (itemsChecked?.size() ?: 0) so the visibility logic uses the true total item
count.
---
Outside diff comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt`:
- Around line 84-89: setState in ListManager updates the adapter and
itemsChecked but never triggers onItemSizeChanged, so undo/redo that changes
item count leaves jump-button visibility stale; after
adapter.submitList(state.items) completes (i.e., inside its completion callback
where you already call focusItem), invoke onItemSizeChanged() to recalculate and
update jump buttons (ensure you do this before or after calling focusItem as
appropriate), and also ensure itemsChecked.setItems(state.checkedItems!!) is
called after this update if ordering matters.
- Around line 367-378: The method deleteCheckedItems reads the adapter-backed
property items after submitting a new list which can be asynchronously swapped;
replace the stale adapter access with the local cloned list: compute the new
size from itemsUpdated and call onItemSizeChanged?.invoke(itemsUpdated.size)
(mirroring the pattern used in delete()), and ensure any other uses that rely on
the post-submit list state use itemsUpdated instead of the adapter-backed items.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt`:
- Around line 644-663: The null-check for getChildAt(0) in the jumpToBottom
button handler is dead code and you can simplify the apply/post block: inside
the addIconButton lambda (the jumpToBottom creation) remove the unreachable else
branch and the explicit binding.ScrollView qualifiers, use the ScrollView
instance referenced by this (from apply) and assume its single child exists
(e.g., val lastChild = getChildAt(0)), compute bottom from lastChild.bottom +
paddingBottom, then call smoothScrollTo(0, bottom); keep
updateJumpButtonsVisibility() as-is.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt`:
- Around line 244-250: The lambda passed to updateJumpButtonsVisibility
currently names its parameter "items", which shadows the EditListActivity.items
property; change the lambda parameter name (e.g., to "itemCount" or "count") so
it reflects that it is an Int and does not shadow the class property, updating
the usage inside the lambda accordingly (the call to updateJumpButtonsVisibility
should remain unchanged).
In
`@app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt`:
- Around line 43-51: The callback parameter name onItemSizeChanged in class
ListManager is misleading—rename the lambda parameter from items to itemCount
(or count) in the ListManager constructor signature and update all call sites
and any anonymous/lambda implementations to use the new name (onItemSizeChanged:
((itemCount: Int) -> Unit)?). Locate references by the symbol onItemSizeChanged
and the ListManager constructor and update parameter documentation/comments if
present.
In
`@app/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt`:
- Around line 61-63: The tests currently pass null for the ListManager
constructor's onItemSizeChanged parameter, so no test can verify the callback;
update ListManagerTestBase to provide a capturing lambda to the ListManager(...)
call (reference: ListManager constructor and listManager variable) that writes
the reported count into a test-scoped var (e.g. lastReportedItemSize)
initialized in the test base, and then update/add tests to assert expected
values after operations (e.g. after listManager.add/... or remove) to validate
the callback is invoked with the correct count.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (10)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/res/drawable/vertical_align_bottom.xmlapp/src/main/res/drawable/vertical_align_top.xmlapp/src/main/res/values/strings.xmlapp/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt
app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt
Show resolved
Hide resolved
app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.kt (1)
249-251: Consider widening the receiver toContextfor broader reusability.
resources.configurationis available on anyContext, so restricting the receiver toActivityis unnecessarily narrow. The current callers inEditActivitywould still compile without change.♻️ Proposed refactor
-val Activity.isInLandscapeMode +val Context.isInLandscapeMode get() = resources.configuration.orientation == Configuration.ORIENTATION_LANDSCAPE🤖 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/AndroidExtensions.kt` around lines 249 - 251, The extension val Activity.isInLandscapeMode is unnecessarily narrow; change its receiver from Activity to Context so any Context can call it (e.g., val Context.isInLandscapeMode). Update the declaration symbol isInLandscapeMode accordingly and ensure usages in EditActivity still compile; keep the implementation using resources.configuration.orientation == Configuration.ORIENTATION_LANDSCAPE and import Configuration if needed.
🤖 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/utils/AndroidExtensions.kt`:
- Around line 249-251: The extension val Activity.isInLandscapeMode is
unnecessarily narrow; change its receiver from Activity to Context so any
Context can call it (e.g., val Context.isInLandscapeMode). Update the
declaration symbol isInLandscapeMode accordingly and ensure usages in
EditActivity still compile; keep the implementation using
resources.configuration.orientation == Configuration.ORIENTATION_LANDSCAPE and
import Configuration if needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (10)
TRANSLATIONS.mdapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.ktapp/src/main/java/com/philkes/notallyx/utils/AndroidExtensions.ktapp/src/main/res/drawable/vertical_align_bottom.xmlapp/src/main/res/drawable/vertical_align_top.xmlapp/src/main/res/values/strings.xmlapp/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.kt
- app/src/main/res/drawable/vertical_align_top.xml
- app/src/main/res/values/strings.xml
- app/src/main/java/com/philkes/notallyx/presentation/activity/note/EditListActivity.kt
- app/src/test/kotlin/com/philkes/notallyx/recyclerview/listmanager/ListManagerTestBase.kt
- app/src/main/res/drawable/vertical_align_bottom.xml
- app/src/main/java/com/philkes/notallyx/presentation/view/note/listitem/ListManager.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 #189
Adds scroll to top/bottom buttons for "long" notes:
Summary by CodeRabbit
New Features
Documentation