app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt (1)

59-72: Replace recursion with a loop to avoid potential stack growth

If many head-of-queue items are non-Pending, the recursive re-fetch can recurse deeply and risk stack overflow. Use a loop.

Apply this diff:

-    fun getPending(order: MessagesSettings.ProcessingOrder): StoredSendRequest? {
-        val message = when (order) {
-            MessagesSettings.ProcessingOrder.LIFO -> dao.getPendingLifo()
-            MessagesSettings.ProcessingOrder.FIFO -> dao.getPendingFifo()
-        } ?: return null
-
-        if (message.state != ProcessingState.Pending) {
-            // if for some reason stored state is not in sync with recipients state
-            dao.updateMessageState(message.message.id, message.state)
-            return getPending(order)
-        }
-
-        return message.toRequest()
-    }
+    fun getPending(order: MessagesSettings.ProcessingOrder): StoredSendRequest? {
+        while (true) {
+            val message = when (order) {
+                MessagesSettings.ProcessingOrder.LIFO -> dao.getPendingLifo()
+                MessagesSettings.ProcessingOrder.FIFO -> dao.getPendingFifo()
+            } ?: return null
+
+            if (message.state != ProcessingState.Pending) {
+                // if for some reason stored state is not in sync with recipients state
+                dao.updateMessageState(message.message.id, message.state)
+                continue
+            }
+
+            return message.toRequest()
+        }
+    }

app/src/main/res/values/arrays.xml (2)

24-31: Mark internal values as non-translatable.

These are persisted/internal values and should not be localized. Mark the values array as non-translatable to prevent accidental translation.

-    <string-array name="processing_order_values">
+    <string-array name="processing_order_values" translatable="false">
         <item>LIFO</item>
         <item>FIFO</item>
     </string-array>

---

24-31: Align resource naming with existing pattern (“titles”/“values”).

Elsewhere in this file you use “…_titles” and “…_values”. For consistency, prefer processing_order_titles instead of processing_order_entries and update usage in preferences.

-    <string-array name="processing_order_entries">
+    <string-array name="processing_order_titles">
         <item>LIFO (newer messages first)</item>
         <item>FIFO (older messages first)</item>
     </string-array>

Apply in preferences:

-            android:entries="@array/processing_order_entries"
+            android:entries="@array/processing_order_titles"

app/src/main/java/me/capcom/smsgateway/data/dao/MessagesDao.kt (2)

31-36: Consider adding an index to support the new ORDER BY path.

Frequent queries filtering by state and ordering by (priority DESC, createdAt ASC/DESC) will benefit from a composite index.

• Suggested Room entity annotation on Message:
  • indices = [Index(value = ["state", "priority", "createdAt"])]
• If you already have indices, ensure they cover (state, priority, createdAt) to avoid full scans.

---

31-36: Add brief KDoc to avoid future LIFO/FIFO mix-ups.

A short comment above each method clarifying the intended order will prevent regressions like this.

Example:

/**
 * FIFO: oldest pending first (priority DESC, createdAt ASC)
 */

app/src/main/res/xml/messages_preferences.xml (1)

21-28: Use app:icon for consistency with other preferences.

This file mixes android:icon and app:icon. Prefer one convention (app:icon is used elsewhere here).

-        <DropDownPreference
-            android:defaultValue="LIFO"
-            android:entries="@array/processing_order_entries"
-            android:entryValues="@array/processing_order_values"
-            android:icon="@drawable/ic_processing_order"
+        <DropDownPreference
+            android:defaultValue="LIFO"
+            android:entries="@array/processing_order_entries"
+            android:entryValues="@array/processing_order_values"
+            app:icon="@drawable/ic_processing_order"
             android:key="messages.processing_order"
             app:title="@string/processing_order_title"
             app:useSimpleSummaryProvider="true" />

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

42-46: Avoid potential ClassCastException by using a safe cast

If a wrong preference type gets associated with one of these keys, (preference as EditTextPreference) will crash. Use a safe cast.

Apply this diff:

-                (preference as EditTextPreference).setOnBindEditTextListener {
+                (preference as? EditTextPreference)?.setOnBindEditTextListener {
                     it.inputType = InputType.TYPE_CLASS_NUMBER
                     it.setSelectAllOnFocus(true)
                     it.selectAll()
                 }

app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesSettings.kt (1)

136-139: Make import of ProcessingOrder more robust

valueOf will throw on unexpected input, potentially breaking imports from older or malformed configs. Consider a tolerant parse that ignores bad values and keeps the default.

Apply this diff:

-                PROCESSING_ORDER -> storage.set(
-                    key,
-                    value?.let { ProcessingOrder.valueOf(it.toString()) })
+                PROCESSING_ORDER -> {
+                    val parsed = value
+                        ?.toString()
+                        ?.uppercase()
+                        ?.let { runCatching { ProcessingOrder.valueOf(it) }.getOrNull() }
+                    if (parsed != null) {
+                        storage.set(key, parsed)
+                    }
+                    // else: keep current/default
+                }

app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt (1)

61-63: Confirm DAO retrieval is atomic to prevent duplicate picks under concurrency

Ensure getPendingLifo/Fifo() either:

• marks/locks the selected row atomically in the same transaction, or
• is called within a transaction that updates state before releasing it.

Otherwise, multiple workers could pick the same message. If not already covered in DAO, consider adding an atomic “claim” step (e.g., update where state=Pending and id IN (…) limit 1 returning …).

I can help propose a Room/SQL pattern for “select-and-claim” in one transaction if needed.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

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

37-43: LIFO query ordering is correct (newest first within priority). Resolved prior concern.

This addresses the earlier review noting swapped directions for FIFO/LIFO.

app/src/main/res/xml/messages_preferences.xml (1)

20-29: Unify icon attribute namespace for consistency.

This file mixes android:icon (Line 6) and app:icon (Line 26). Prefer one convention across the file (typically android:icon for Preference icon), unless there’s a project-wide guideline to use app:icon.

Proposed change:

-            app:icon="@drawable/ic_processing_order"
+            android:icon="@drawable/ic_processing_order"

app/src/main/java/me/capcom/smsgateway/modules/messages/repositories/MessagesRepository.kt (4)

12-12: Avoid repository dependency on UI/settings layer types.

Taking MessagesSettings.ProcessingOrder couples the repository to the settings module. Consider introducing a domain-level enum (e.g., ProcessingOrder in the messages domain) and map the settings value at the boundary (e.g., in the service). This keeps repository free of settings concerns.

Additional change outside this hunk (illustrative):

// domain-level
enum class ProcessingOrder { LIFO, FIFO }

// in repository
fun getPending(order: ProcessingOrder): StoredSendRequest? { /* ... */ }

// in service (map from settings enum to domain enum)
val order = when (settings.processingOrder) {
    MessagesSettings.ProcessingOrder.LIFO -> ProcessingOrder.LIFO
    MessagesSettings.ProcessingOrder.FIFO -> ProcessingOrder.FIFO
}
repo.getPending(order)

---

59-74: Convert recursion to loop: good; consider a bounded guard to prevent edge-case spins.

The while(true) + resync logic is sound. For robustness against unexpected data conditions (e.g., update not taking effect, stale caches), add a small retry cap.

Apply:

-    fun getPending(order: MessagesSettings.ProcessingOrder): StoredSendRequest? {
-        while (true) {
+    fun getPending(order: MessagesSettings.ProcessingOrder): StoredSendRequest? {
+        var attempts = 0
+        val maxAttempts = 100
+        while (attempts++ < maxAttempts) {
             val message = when (order) {
                 MessagesSettings.ProcessingOrder.LIFO -> dao.getPendingLifo()
                 MessagesSettings.ProcessingOrder.FIFO -> dao.getPendingFifo()
             } ?: return null
 
             if (message.state != ProcessingState.Pending) {
                 // if for some reason stored state is not in sync with recipients state
                 dao.updateMessageState(message.message.id, message.state)
                 continue
             }
 
             return message.toRequest()
         }
+        return null
     }

---

59-74: Add unit tests for both FIFO and LIFO paths.

Given the new branching on order, add tests to ensure:

• For equal priority, FIFO returns the oldest createdAt; LIFO returns the newest.
• Priority DESC is honored before createdAt.

I can draft DAO-level tests inserting messages with varying priority/createdAt, then asserting the expected id is returned by each method. Want me to open an issue and provide a test skeleton?

---

59-74: Double-check concurrent retrieval semantics.

If multiple workers call getPending concurrently, consider whether you need to atomically transition the selected message from Pending to, say, Processing to avoid duplicate processing. SQLite doesn’t support SKIP LOCKED; a common pattern is to update the row’s state in the same transaction as selection. Current code reads, then updates only on mismatch. If concurrency is possible, you may want a DAO method that atomically claims a pending message.

I can propose a transactional “claim next” DAO method (SELECT … FOR UPDATE emulation via an UPDATE with matching ORDER BY using a subquery) tailored to SQLite/Room if needed.

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

30-43: Consider adding a composite index to support these queries.

To keep getPendingFifo/Lifo fast as data grows, add an index on (state, priority DESC, createdAt ASC/DESC). In Room, define it on the Message entity:

Example (outside this file):

// In Message entity
@Entity(
  tableName = "message",
  indices = [
    Index(value = ["state", "priority", "createdAt"])
  ]
)
data class Message( /* ... */ )

If priority is low cardinality, the state + createdAt index may suffice; measure with EXPLAIN QUERY PLAN.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayApi.kt (1)

222-232: Avoid overloading toString() for wire values.

Overriding toString() for transport can surprise logs/telemetry. Prefer an explicit wireValue property or function.

Option A (keep as-is): Acceptable if toString is only used for query serialization.

Option B (preferred): Add an explicit property and use it in getMessages:

enum class ProcessingOrder(val wireValue: String) {
    @SerializedName("lifo") LIFO("lifo"),
    @SerializedName("fifo") FIFO("fifo"),
}

Then pass parameter("order", processingOrder.wireValue).

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (1)

197-202: Add backward-compatible fallback for servers that don’t support the ‘order’ query

Older servers will 400/404 when ?order= is present. Wrap the two-arg call and fall back to the legacy endpoint. This mirrors the earlier review guidance and is still missing here.

Apply this diff to introduce the fallback:

-        val messages = api.getMessages(settings.token, processingOrder)
+        val messages = try {
+            api.getMessages(settings.token, processingOrder)
+        } catch (e: ClientRequestException) {
+            when (e.response.status) {
+                HttpStatusCode.BadRequest, HttpStatusCode.NotFound -> {
+                    // Older servers don’t support 'order' yet; fall back to legacy endpoint
+                    api.getMessages(settings.token)
+                }
+                else -> throw e
+            }
+        }

Note: This assumes a single-argument overload getMessages(token: String) exists in GatewayApi. If not present, please add it.

To verify the overload exists and is used only via this fallback pattern, run:

#!/usr/bin/env bash
set -euo pipefail

echo "Search GatewayApi for getMessages overloads"
rg -n --no-heading $'interface\\s+GatewayApi' -A200 | rg -n --no-heading $'getMessages\\(' -A1

echo
echo "Search call sites of getMessages"
rg -n --no-heading $'getMessages\\(' -A1 -B1

app/src/main/java/me/capcom/smsgateway/modules/gateway/GatewayService.kt (1)

195-201: Nit: avoid shadowing the class property ‘settings’ with a local ‘settings’

The local val settings = settings.registrationInfo ?: return shadows the class property and reduces readability. Prefer a distinct name like registration or regInfo and update the use sites (Line 201).

Example (outside selected lines):

val registration = settings.registrationInfo ?: return
...
val messages = api.getMessages(registration.token, processingOrder)

app/src/main/java/me/capcom/smsgateway/modules/messages/MessagesRepository.kt (1)

58-63: Decouple repository from settings by using a domain-level ProcessingOrder

The repository shouldn’t depend on a UI/settings type. Introduce a domain-level ProcessingOrder (e.g., in me.capcom.smsgateway.domain) and map UI settings to it at the boundary. This prevents cross-layer coupling and avoids drift with other enums (e.g., GatewayApi.ProcessingOrder).

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro