app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt (1)

49-65: Consider using visual selection indicators instead of disabling buttons.

Using isEnabled = false to indicate the active tab causes the selected button to appear grayed out, which typically communicates "unavailable" rather than "selected." Consider using isSelected = true with a custom selector drawable, or switching to MaterialButtonToggleGroup for standard toggle behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt` around lines 49
- 65, The current selectOutgoing and selectIncoming use isEnabled to indicate
the active tab which grays out the button; instead set visual state via
isSelected (e.g., binding.buttonOutgoing.isSelected = true / isSelected = false)
and remove the isEnabled toggles, and update the button backgrounds to use a
selector drawable (or migrate the two buttons into a MaterialButtonToggleGroup)
so selection is shown visually while keeping the fragment swaps in
selectOutgoing and selectIncoming (referencing the methods
selectOutgoing/selectIncoming and the views binding.buttonOutgoing and
binding.buttonIncoming).

app/src/main/java/me/capcom/smsgateway/ui/IncomingMessagesListFragment.kt (1)

65-73: Minor: scroll listener could trigger unnecessary calls on empty list.

When the RecyclerView is empty, findLastVisibleItemPosition() returns -1 and adapter.itemCount - 1 is also -1, so the condition matches. While loadMore(0) safely returns early in the ViewModel, this creates unnecessary calls.

Consider adding a guard:

♻️ Optional improvement

     private val scrollListener = object : RecyclerView.OnScrollListener() {
         override fun onScrolled(recyclerView: RecyclerView, dx: Int, dy: Int) {
             super.onScrolled(recyclerView, dx, dy)
+            if (adapter.itemCount == 0) return
             val manager = recyclerView.layoutManager as? LinearLayoutManager
             manager?.findLastVisibleItemPosition()?.let {
                 if (it == adapter.itemCount - 1) viewModel.loadMore(adapter.itemCount)
             }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/IncomingMessagesListFragment.kt`
around lines 65 - 73, The scroll listener (scrollListener -> onScrolled) can
call viewModel.loadMore unnecessarily when the list is empty because
findLastVisibleItemPosition() and adapter.itemCount - 1 both equal -1; update
the onScrolled guard to first check that adapter.itemCount > 0 and that the
found lastVisible (from manager?.findLastVisibleItemPosition()) is >= 0 before
comparing it to adapter.itemCount - 1, and only call
viewModel.loadMore(adapter.itemCount) when both conditions are met.

INCOMING_MESSAGES_PLAN.md (1)

134-140: Tests described in plan but not implemented.

The PR summary indicates "No new automated tests added for the incoming module," yet this section outlines comprehensive test coverage expectations. Consider adding at least critical-path tests (DAO unit tests for stats/pagination, receiver integration test) or creating a follow-up issue to track test debt.

Would you like me to open an issue to track the missing test coverage for the incoming module?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@INCOMING_MESSAGES_PLAN.md` around lines 134 - 140, The plan lists
comprehensive tests that were not implemented; either add the critical-path
tests or create a follow-up issue tracking the missing coverage. Specifically,
implement unit tests for the DAO (e.g., IncomingDao.statsQuery and pagination
behavior), repository tests for mapping and totals (IncomingRepository.map* /
getTotals), an integration test for the receiver to assert persistence before
webhook dispatch (IncomingReceiver.handleWebhook), a Fragment/ViewModel test for
stats rendering and load-more (IncomingFragment / IncomingViewModel), and a
migration test for old DB upgrade (MigrationUpgradeOldDb); if you won't add them
now, create a tracked issue outlining these exact tests with priorities and
acceptance criteria.

app/src/main/java/me/capcom/smsgateway/modules/incoming/vm/IncomingMessagesListViewModel.kt (1)

24-31: Redundant loadMore() call in init block.

The loadMore() call on line 30 is dead code. Since limit is initialized with chunkSize (50), the switchMap on line 25 already triggers repository.selectLast(50) immediately. When loadMore() is called with default index=0, the condition currentLimit >= index + chunkSize evaluates to 50 >= 50, returning early without doing anything.

Remove the redundant call:

♻️ Remove dead code

     init {
         _messages.addSource(limit.switchMap { repository.selectLast(it) }) {
             _messages.value = it
             hasMore = it.size >= (limit.value ?: chunkSize)
             isLoading = false
         }
-        loadMore()
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/incoming/vm/IncomingMessagesListViewModel.kt`
around lines 24 - 31, The init block currently calls loadMore() redundantly
because limit is initialized to chunkSize so the switchMap on limit already
triggers repository.selectLast(chunkSize); remove the dead call to loadMore()
from the init block (the block that sets up _messages.addSource(limit.switchMap
{ repository.selectLast(it) }) and the subsequent hasMore/isLoading updates) so
initialization relies on the limit switchMap; leave the other logic (hasMore,
isLoading updates and loadMore function) unchanged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt (1)

74-77: Tab switches recreate fragments and lose RecyclerView scroll position.

Using replace(...) on lines 75 and 86 destroys and recreates the inactive tab's fragment. Since neither MessagesListFragment nor IncomingMessagesListFragment persist RecyclerView position, switching tabs resets the scroll state each time. Consider keeping both child fragments and using show/hide by tag instead to preserve instance state and scroll position.

♻️ Proposed refactor (preserve child fragment instances)

 class HolderFragment : Fragment() {
+    private companion object {
+        const val TAG_OUTGOING = "tab_outgoing"
+        const val TAG_INCOMING = "tab_incoming"
+    }
+
     private fun selectOutgoing() {
         isOutgoingSelected = true
-
-        binding.buttonOutgoing.isEnabled = false
-        binding.buttonIncoming.isEnabled = true
-
-        childFragmentManager.commit {
-            replace(R.id.rootLayout, MessagesListFragment.newInstance())
-        }
+        updateButtonStates()
+        val outgoing = childFragmentManager.findFragmentByTag(TAG_OUTGOING)
+            ?: MessagesListFragment.newInstance()
+        val incoming = childFragmentManager.findFragmentByTag(TAG_INCOMING)
+
+        childFragmentManager.commit {
+            if (!outgoing.isAdded) add(R.id.rootLayout, outgoing, TAG_OUTGOING)
+            incoming?.let { hide(it) }
+            show(outgoing)
+        }
     }
 
     private fun selectIncoming() {
         isOutgoingSelected = false
-
-        binding.buttonOutgoing.isEnabled = true
-        binding.buttonIncoming.isEnabled = false
-
-        childFragmentManager.commit {
-            replace(R.id.rootLayout, IncomingMessagesListFragment.newInstance())
-        }
+        updateButtonStates()
+        val incoming = childFragmentManager.findFragmentByTag(TAG_INCOMING)
+            ?: IncomingMessagesListFragment.newInstance()
+        val outgoing = childFragmentManager.findFragmentByTag(TAG_OUTGOING)
+
+        childFragmentManager.commit {
+            if (!incoming.isAdded) add(R.id.rootLayout, incoming, TAG_INCOMING)
+            outgoing?.let { hide(it) }
+            show(incoming)
+        }
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt` around lines 74
- 77, The current use of childFragmentManager.commit with
replace(R.id.rootLayout, MessagesListFragment.newInstance()) (and the analogous
replace for IncomingMessagesListFragment) recreates fragments and loses
RecyclerView scroll state; change the logic in HolderFragment to add both child
fragments once using add(..., tag) (use MessagesListFragment.newInstance() and
IncomingMessagesListFragment.newInstance()), then toggle visibility with
show(tag)/hide(tag) instead of replace, ensuring you reuse existing fragments by
finding them via childFragmentManager.findFragmentByTag(tag) so instances (and
RecyclerView position) are preserved across tab switches.

app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt (2)

43-65: Add a recreate/switch regression test for this flow.

This fragment now has real UI state, saved-state restore, and two child-fragment paths. A focused FragmentScenario test that selects incoming, recreates the fragment, and asserts the restored button/child state would make this much safer to evolve.

Also applies to: 72-99

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt` around lines 43
- 65, Add a FragmentScenario recreation test that exercises the
saved-state/child-fragment flow: use HolderFragment via FragmentScenario, call
the fragment methods (or drive UI) to selectIncoming (instead of default
selectOutgoing), call scenario.recreate() to simulate process death, then assert
isOutgoingSelected is false and that updateButtonStates reflects the Incoming
button selected and the Incoming child fragment is attached; reference
HolderFragment's onViewCreated, selectIncoming/selectOutgoing,
isOutgoingSelected, KEY_OUTGOING_SELECTED and updateButtonStates to locate the
state and verify the child fragment in the test.

---

67-69: Don’t use isEnabled as the selected-tab state.

These two Buttons in app/src/main/res/layout/fragment_holder.xml:15-36 are acting like tabs, but the active one is exposed as disabled instead of selected. That makes the current tab harder to understand for accessibility services and keyboard/DPAD users. Prefer isSelected/isActivated with stateful styling, or a real tab/toggle component, and keep both controls enabled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt` around lines 67
- 69, The updateButtonStates function is using isEnabled to indicate the active
tab; change it to set the visual/selection state instead: keep both
binding.buttonOutgoing and binding.buttonIncoming enabled (isEnabled = true) and
set their isSelected (or isActivated) according to isOutgoingSelected (e.g.,
binding.buttonOutgoing.isSelected = isOutgoingSelected;
binding.buttonIncoming.isSelected = !isOutgoingSelected). Update any click
handlers to still toggle isOutgoingSelected and ensure your selector/stateful
styling in fragment_holder.xml responds to the selected/activated state for
proper keyboard/DPAD and accessibility behavior.

app/src/main/java/me/capcom/smsgateway/modules/incoming/vm/IncomingMessagesListViewModel.kt (1)

24-31: Redundant loadMore() call in init block.

The loadMore() call on line 30 is effectively a no-op. Since limit is initialized to chunkSize (50), when loadMore(0) executes, the condition currentLimit >= index + chunkSize evaluates to 50 >= 50, causing an early return. The initial data load is already triggered by the switchMap when it observes the initial limit value.

Proposed fix

     init {
         _messages.addSource(limit.switchMap { repository.selectLast(it) }) {
             _messages.value = it
             hasMore = it.size >= (limit.value ?: chunkSize)
             isLoading = false
         }
-        loadMore()
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/incoming/vm/IncomingMessagesListViewModel.kt`
around lines 24 - 31, The init block calls loadMore() redundantly because
_messages.addSource already triggers a load for the initial limit; remove the
loadMore() invocation in the init block (the call to loadMore() after the
_messages.addSource(...) closure) so the initial dataset is loaded only via the
limit.switchMap observer; verify behavior using the limit, chunkSize, hasMore
and isLoading variables in IncomingMessagesListViewModel to ensure no other
logic depends on that explicit call.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/ui/IncomingMessagesListFragment.kt (1)

36-40: Set the LinearLayoutManager explicitly in code.

Line 72 assumes a LinearLayoutManager, but this fragment never sets one directly. Making it explicit avoids silent pagination breakage if the XML manager changes.

Suggested change

         binding.recyclerView.adapter = adapter
+        binding.recyclerView.layoutManager = LinearLayoutManager(requireContext())
         binding.recyclerView.addOnScrollListener(scrollListener)
         binding.recyclerView.addItemDecoration(
             DividerItemDecoration(requireContext(), DividerItemDecoration.VERTICAL)
         )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/IncomingMessagesListFragment.kt`
around lines 36 - 40, The fragment attaches an adapter and a scrollListener to
binding.recyclerView but never sets a LayoutManager, yet scrollListener assumes
a LinearLayoutManager; explicitly set binding.recyclerView.layoutManager =
LinearLayoutManager(requireContext()) (or the desired orientation) before
assigning adapter/scrollListener to ensure pagination (scrollListener) works
even if the XML changes; reference binding.recyclerView, scrollListener, adapter
and use LinearLayoutManager in the fragment initialization block where the
RecyclerView is configured.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/modules/incoming/db/IncomingMessagesDao.kt (1)

14-15: Make the "latest" query deterministic.

Line 14 orders only by createdAt, so rows with the same timestamp can shuffle between emissions and make the LIMIT :limit window unstable. Add a unique tie-breaker.

♻️ Suggested query tweak

-    `@Query`("SELECT * FROM incoming_messages ORDER BY createdAt DESC LIMIT :limit")
+    `@Query`("SELECT * FROM incoming_messages ORDER BY createdAt DESC, id DESC LIMIT :limit")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@app/src/main/java/me/capcom/smsgateway/modules/incoming/db/IncomingMessagesDao.kt`
around lines 14 - 15, The SELECT in function selectLast uses ORDER BY createdAt
DESC which is non-deterministic when multiple rows share the same timestamp;
update the query to include a unique tie-breaker (e.g., add the primary key
column such as id) to the ORDER BY clause so results are stable across emissions
(for example ORDER BY createdAt DESC, id DESC), ensuring selectLast(limit: Int)
returns a deterministic window.

app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt (1)

36-55: Add a migration test for 18 → 19.

This upgrade path now runs for every existing install. A Room migration test against a real v18 schema is the safest way to catch startup-time failures from the new table/indexes before release.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/data/AppDatabase.kt` around lines 36 -
55, Add a Room migration test that verifies the AppDatabase upgrade from schema
version 18 to version 19 using the same database schema and migrations
configured in AppDatabase (version = 19 and AutoMigration list); write a test
(e.g., AppDatabaseMigrationTest) that uses MigrationTestHelper (or
SupportSQLite) to create a real v18 database (using the v18 schema SQL asset or
a pre-populated file), run the migrations applied by AppDatabase (including the
AutoMigration from 18 to 19), and assert the database opens and expected new
tables/indexes/columns exist and queries succeed; ensure the test references
AppDatabase and the migration path 18→19 so future changes fail the test if the
migration is broken.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt (1)

72-85: Consider avoiding show() / hide() for tab switching with these list fragments.

show() / hide() keeps the hidden fragment's view lifecycle active. Both MessagesListFragment.kt and IncomingMessagesListFragment.kt attach LiveData observers and RecyclerView scroll listeners in onViewCreated(), so the inactive tab continues receiving list updates and scroll events while off-screen. If that extra work isn't intentional, use replace() or gate the observers/pagination via onHiddenChanged() instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/main/java/me/capcom/smsgateway/ui/HolderFragment.kt` around lines 72
- 85, selectOutgoing currently uses childFragmentManager.commit with
show()/hide(), which keeps the hidden fragment's view lifecycle active and lets
MessagesListFragment and IncomingMessagesListFragment continue receiving
LiveData updates and scroll events; change the fragment transaction to use
replace(...) for TAG_OUTGOING (remove show/hide and add/replace the outgoing
fragment) so only the active fragment has its view lifecycle, or alternatively
modify MessagesListFragment and IncomingMessagesListFragment to gate
observer/scroll listener attachment in
onHiddenChanged()/setMenuVisibility()/onResume() so observers are only active
when visible (update selectOutgoing to stop calling hide/show and update
childFragmentManager.commit accordingly).