2.7.13

[ianrumac]

Changes in this pull request

Checklist

• All unit tests pass.
• All UI tests pass.
• Demo project builds and runs.
• I added/updated tests or detailed why my change isn't tested.
• I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
• I have run ktlint in the main directory and fixed any issues.
• I have updated the SDK documentation as well as the online docs.
• I have reviewed the contributing guide

Greptile Summary

This PR refactors the config subsystem to use an actor-based state machine (ConfigState.Actions sealed class) running through a SequentialActor, consolidates TestModeManager into the new TestMode class with injected UI/network hooks, fixes R8 minification by replacing polymorphic kotlinx.serialization on Capability with hand-built JSON, and adds Locale.US guards in DeviceHelper.asPadded(). All P-level findings are P2 style or fragility notes; no blocking issues were found.

Confidence Score: 4/5

Safe to merge — no P0/P1 issues found; all findings are P2 style or fragility notes.

Significant refactor with comprehensive new tests (ConfigManagerTest is 1,588 lines). All identified issues are P2: a dead try-catch, an unchecked cast without @Suppress, a potential Int overflow on a very large Duration, and fragile positional destructuring of awaitAll(). None affect correctness in realistic conditions.

superwall/src/main/java/com/superwall/sdk/config/models/ConfigState.kt (FetchConfig action is dense; positional awaitAll destructuring is the most fragile spot)

Important Files Changed

Filename	Overview
superwall/src/main/java/com/superwall/sdk/config/models/ConfigState.kt	Config state now owns all action logic (FetchConfig, ApplyConfig, RefreshConfig, etc.) via nested sealed classes; fragile positional awaitAll() destructuring flagged.
superwall/src/main/java/com/superwall/sdk/config/ConfigManager.kt	ConfigManager now implements ConfigContext; heavy logic moved to ConfigState.Actions; actor-based state management replaces MutableStateFlow.
superwall/src/main/java/com/superwall/sdk/store/testmode/TestMode.kt	Replaces TestModeManager; merges activation UI flow, product refresh, and state management into a single injectable class; redundant outer try-catch flagged in isTestEnvironment.
superwall/src/main/java/com/superwall/sdk/network/device/Capability.kt	Replaced kotlinx.serialization polymorphic encoding with hand-built JSON to fix R8 minification reflection issues; wire format is backward-compatible (both 'type' and 'name' fields preserved).
superwall/src/main/java/com/superwall/sdk/network/RequestExecutor.kt	Adds optional connection/read timeout support; Int overflow risk on very large Duration values flagged.
superwall/src/main/java/com/superwall/sdk/network/device/DeviceHelper.kt	String.format calls now use Locale.US to avoid locale-specific number formatting in asPadded(); capabilities serialization updated to use new toJson().
superwall/src/main/java/com/superwall/sdk/misc/Either.kt	Adds thenIf() extension for conditional execution in Either pipelines; unchecked cast without @Suppress annotation flagged.
superwall/src/main/java/com/superwall/sdk/dependencies/DependencyContainer.kt	Wires the new TestMode and configActor; TestMode now receives all UI/network dependencies at construction; options variable moved earlier.

Comments Outside Diff (1)

1. superwall/src/main/java/com/superwall/sdk/store/testmode/TestMode.kt, line 55-71 (link)

   Unreachable outer try-catch in isTestEnvironment

   The outer try { ... } catch (_: ClassNotFoundException) can never trigger because every Class.forName call is already wrapped in runCatching, which catches all Throwables including ClassNotFoundException. The outer handler is dead code and may confuse future readers into thinking there is an unguarded exception path.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: superwall/src/main/java/com/superwall/sdk/store/testmode/TestMode.kt
   Line: 55-71
   
   Comment:
   **Unreachable outer `try-catch` in `isTestEnvironment`**
   
   The outer `try { ... } catch (_: ClassNotFoundException)` can never trigger because every `Class.forName` call is already wrapped in `runCatching`, which catches all `Throwable`s including `ClassNotFoundException`. The outer handler is dead code and may confuse future readers into thinking there is an unguarded exception path.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
superwall/src/main/java/com/superwall/sdk/store/testmode/TestMode.kt:55-71
**Unreachable outer `try-catch` in `isTestEnvironment`**

The outer `try { ... } catch (_: ClassNotFoundException)` can never trigger because every `Class.forName` call is already wrapped in `runCatching`, which catches all `Throwable`s including `ClassNotFoundException`. The outer handler is dead code and may confuse future readers into thinking there is an unguarded exception path.

### Issue 2 of 4
superwall/src/main/java/com/superwall/sdk/network/RequestExecutor.kt:146-147
**Timeout `0` means infinite; overflow on large `Duration` values**

`HttpURLConnection.connectTimeout = 0` means **no timeout** (blocks indefinitely), which mirrors the pre-PR behavior — fine as a default. However, `inWholeMilliseconds.toInt()` silently overflows for any `Duration` > ~24.8 days, producing a negative or truncated timeout instead of an error. A `coerceIn` guard would make the intent explicit:

```suggestion
        connection.connectTimeout = timeout?.inWholeMilliseconds?.coerceIn(0, Int.MAX_VALUE.toLong())?.toInt() ?: 0
        connection.readTimeout = timeout?.inWholeMilliseconds?.coerceIn(0, Int.MAX_VALUE.toLong())?.toInt() ?: 0
```

### Issue 3 of 4
superwall/src/main/java/com/superwall/sdk/misc/Either.kt:50-56
**Unchecked `as E` cast without `@Suppress`**

`IllegalStateException(...) as E` is an unchecked cast. The cast is safe at runtime due to type erasure (E is bounded by `Throwable`), but the compiler warning should be suppressed explicitly to match the pattern used in cast-heavy sections of `ConfigState`.

```suggestion
            } catch (e: Throwable) {
                @Suppress("UNCHECKED_CAST")
                (e as? E)?.let { Either.Failure(it) }
                    ?: Either.Failure(IllegalStateException("Error in then block", e) as E)
            }
```

### Issue 4 of 4
superwall/src/main/java/com/superwall/sdk/config/models/ConfigState.kt:178-185
**Positional destructuring of heterogeneous `awaitAll()` result is fragile**

`listOf(configDeferred, enrichmentDeferred).awaitAll()` returns `List<Any?>`. The result is positionally destructured and unsafely cast to typed `Either` values. If the two `Deferred`s are ever reordered, the casts would silently swap `configResult` and `enrichmentResult`, producing NPEs or wrong data downstream without a compile-time warning. Consider awaiting them independently:

```kotlin
val configResultRaw = configDeferred.await()
val enrichmentResult = enrichmentDeferred.await()
val attributes = attributesDeferred.await()
```

_{Reviews (1): Last reviewed commit: "Merge pull request #402 from superwall/i..." | Re-trigger Greptile}

Greptile also left 3 inline comments on this PR.