[TV] Fix search failing on unknown combined-search result types

[sztomek]

Description

Search hard-fails whenever the /search/combined response contains a result the client doesn't recognise. The response is decoded with a Moshi PolymorphicJsonAdapterFactory keyed on type, registered only for podcast and episode. The server also returns "type":"network" results (podcast networks), and the factory throws JsonDataException: Expected one of [podcast, episode] for key 'type' but found 'network' — which aborts decoding of the entire response, so a search that returned perfectly good podcasts and episodes surfaces as a generic error with zero results.

This reproduces for common queries (e.g. news, search) and affects both phone and TV search (both go through ImprovedSearchManager.combinedSearch). Verified live: POST /search/combined returns HTTP 200 with valid results, yet the app shows the error state, with this stacktrace:

com.squareup.moshi.JsonDataException: Expected one of [podcast, episode] for key 'type' but found 'network'.
  at ...CombinedSearchResponseJsonAdapter.fromJson(CombinedSearchResponse.kt)

Fix

Add an Unknown fallback to the polymorphic adapter (withDefaultValue(Unknown)) so any unrecognised type (today network, and any future type) decodes to CombinedResult.Unknown instead of throwing. ImprovedSearchManagerImpl.combinedSearch already uses mapNotNull, so unknown results are simply dropped and the rest of the results render normally.

Testing Instructions

1. Search for a term that returns a network (e.g. news) on phone or TV → results now appear instead of an error.
2. Unit tests: ./gradlew :modules:services:servers:testDebugUnitTest --tests "*CombinedSearchResponseTest*" :modules:services:repositories:testDebugUnitTest --tests "*ImprovedSearchManagerImplTest*" — covers the adapter tolerating a network result and the mapping dropping Unknown.

Screenshots or Screencast

Before fix	After fix
Screen_recording_20260814_175406.mp4	Screen_recording_20260814_175445.mp4

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md — N/A (bug fix, no user-facing string)
• Ensure the linter passes (./gradlew spotlessApply)
• I have considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in localization — N/A
• Any jetpack compose components I added or changed are covered by compose previews — N/A
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics — N/A

[claude]

Claude finished @sztomek's task in 2m 36s —— View job

---

Code review

The diagnosis and the fix are both right. PolymorphicJsonAdapterFactory throws from labelIndex() on an unregistered label, and because the throw happens inside CombinedSearchResponseJsonAdapter's list loop, one "type":"network" entry aborts decoding of the whole payload — matching the reported "HTTP 200 with good results, UI shows error state". withDefaultValue(Unknown) is the idiomatic Moshi remedy, and ImprovedSearchManagerImpl already used mapNotNull, so the drop is a one-line when branch rather than a restructure. The when stays exhaustive over the sealed interface, so any future subtype is a compile error rather than a silent miss — good shape.

Two inline comments posted. Summary of everything I found:

1. AutoCompleteResult has the identical latent bug — worth fixing here (medium)

modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/search/AutoCompleteResponse.kt:23-25:

val jsonAdapter = PolymorphicJsonAdapterFactory.of(AutoCompleteResult::class.java, "type")
    .withSubtype(TermResult::class.java, "term")
    .withSubtype(PodcastResult::class.java, "podcast")
    // no .withDefaultValue(...)

Same factory, same "type" discriminator, same search feature, same ImprovedSearchManagerImpl — and the same server that just started returning network on /search/combined. If autocomplete ever gains a type, the search-as-you-type box hard-fails exactly the way combined search does today. Fixing it in this PR is ~4 lines (data object Unknown, withDefaultValue, map → mapNotNull, one test) and closes the class of bug rather than one instance of it.

Fix this →

2. Required non-null fields keep the whole-response failure mode alive (low/medium)

Detailed inline. Short version: PodcastResult.slug and EpisodeResult.{title,url,podcastUuid,podcastTitle,podcastSlug} are non-null with no defaults, so a podcast result missing slug still throws Required value 'slug' missing at $.results[N] and nukes the entire search — same symptom, different trigger. slug and podcastSlug in particular are never read by any consumer, so the strictness buys nothing. Reasonable as a follow-up.

3. Unknown results are dropped with zero observability (low)

Nothing logs when a result is discarded. If the server broadens network results (or adds another type), the client quietly returns a shorter list and no signal reaches anyone. A LogBuffer.i in the Unknown branch — or an analytics counter — would make the next occurrence visible instead of invisible. Related product question, probably out of scope here: network results are a real, intentional server feature, so dropping them means podcast networks never surface in search. Worth a tracking issue if there isn't one.

Smaller notes

• Test placement (src/test/kotlin in servers, src/test/java in repositories) matches each module's existing convention. ✅
• CombinedSearchResponseTest hand-builds a Moshi that mirrors a subset of NetworkModule.provideMoshi; the Rfc3339DateJsonAdapter line is genuinely needed because the polymorphic factory resolves subtype adapters eagerly. Fine as-is; suggested two extra cases inline.
• No security or performance implications — no new I/O, allocation, or parsing work on any hot path; if anything this removes an exception throw.

Verification

I could not run ./gradlew in this environment (the command required approval that isn't granted to this job), so the two test tasks in the PR description are unverified by me. Reading them, both assertions follow from the code as written. Add Bash(./gradlew*) to --allowedTools if you'd like future reviews to actually execute the suite.

Verdict: the fix is correct and well-tested for what it targets. Item 1 is the one I'd genuinely like to see folded in — same bug, same file directory, same PR. Items 2 and 3 are fine as follow-ups.
• branch fix/tv-search-network-crash

[claude]

Claude finished @sztomek's task in 2m 20s —— View job

---

Code review (re-run on ready for review)

The fix is correct and I'd ship it. PolymorphicJsonAdapterFactory throws from labelIndex() on an unregistered label, and because that throw happens inside the generated CombinedSearchResponseJsonAdapter list loop, one "type":"network" entry aborts decoding of the entire payload — exactly matching "HTTP 200 with good results, UI shows error state". withDefaultValue(Unknown) installs a fallback adapter that skipValue()s the unrecognised object and returns Unknown, so decoding continues; ImprovedSearchManagerImpl already used mapNotNull, making the drop a one-line when branch. The when stays exhaustive over the sealed interface, so a future subtype is a compile error rather than a silent miss. Good shape.

One correction to my earlier review, since it changes what you might do:

⚠️ withDefaultValue does not cover a missing type key (correcting my earlier inline comment)

I previously said the fallback also kicks in when type is absent entirely, and suggested a test for it. That's wrong for Moshi 1.15.2 — labelIndex() returns -1 (→ fallback) only for an unrecognised label; when the loop finishes without ever seeing the label key it throws JsonDataException("Missing label for type") unconditionally, not guarded by fallbackJsonAdapter. So don't add that test — it would fail. Detail inline. Not a problem in practice (the server always sends type), but the fix's blast radius is unknown-label only.

---

Findings

1. AutoCompleteResult has the identical latent bug — worth folding in here (medium)

modules/services/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/search/AutoCompleteResponse.kt:22-26 (unchanged by this PR):

val jsonAdapter = PolymorphicJsonAdapterFactory.of(AutoCompleteResult::class.java, "type")
    .withSubtype(TermResult::class.java, "term")
    .withSubtype(PodcastResult::class.java, "podcast")
    // no .withDefaultValue(...)

Same factory, same "type" discriminator, same feature, registered side-by-side in NetworkModule.kt:101-102, and served by the same backend that just started returning network on /search/combined. If autocomplete gains a type, search-as-you-type hard-fails the way combined search does today. ImprovedSearchManagerImpl.autoCompleteSearch (ImprovedSearchManagerImpl.kt:19) uses map, so it'd need mapNotNull too — about 4 lines plus a test, and it closes the class of bug rather than one instance.

Fix this →

2. Required non-null fields keep the same whole-response failure mode alive (low/medium)

Detailed inline. PodcastResult.slug and EpisodeResult.{title,url,podcastUuid,podcastTitle,podcastSlug} are non-null with no defaults (CombinedSearchResponse.kt:19,25-36), so one podcast result missing slug still throws Required value 'slug' missing at $.results[N] and nukes the whole search — same symptom, different trigger. slug and podcastSlug are never read by any consumer (ImprovedSearchManagerImpl.kt:37-55 ignores them; no other consumers of CombinedResult), so requiring them buys nothing. Fine as a follow-up.

3. Unknown results are dropped with zero observability (low)

Nothing logs a discard. If the server broadens network results or adds a type, the client silently returns a shorter list and no signal reaches anyone. A LogBuffer.i in the Unknown branch would make the next occurrence visible. Separate product question, out of scope here: network results are an intentional server feature, so dropping them means podcast networks never surface in search — worth a tracking issue if one doesn't exist.

Smaller notes

• PolymorphicJsonAdapter.toJson throws IllegalArgumentException for a value whose class isn't a registered subtype, so serialising CombinedResult.Unknown would fail. Nothing does today (CombinedSearchResponse is response-only, and OkHttp caches raw bytes), so this is informational, not a bug.
• Test placement (src/test/kotlin in servers, src/test/java in repositories) matches each module's existing convention ✅.
• The hand-built Moshi in CombinedSearchResponseTest genuinely needs the Rfc3339DateJsonAdapter line — the polymorphic factory resolves subtype adapters eagerly, so EpisodeResult's Date field must be resolvable even when the payload has no episodes.
• No security or performance implications: no new I/O, allocation, or parsing on any hot path; if anything this removes an exception throw.

Verification

I could not run ./gradlew — the command requires approval this job doesn't have, and there's no populated Gradle cache in the runner, so both test tasks from the PR description are unverified by me. Reading them, the assertions follow from the code as written. Add Bash(./gradlew*) to --allowedTools if you'd like future reviews to execute the suite. The Moshi claim above is from the 1.15.2 PolymorphicJsonAdapter source rather than an executed test — cheap to confirm if you care.

Verdict: correct, well-scoped fix with a regression test that reproduces the reported payload. Item 1 is the one I'd like folded in — same bug, same directory, same PR. Items 2 and 3 are fine as follow-ups. No changes pushed; this is review feedback only.

---

• Gather context (PR diff vs origin/main)
• Read changed files + surrounding search code
• Check for the same pattern elsewhere (autocomplete, widget adapter)
• Verify Moshi fallback semantics against 1.15.2 behaviour
• Attempt to run tests — blocked, see Verification
• Post review
  • branch fix/tv-search-network-crash

[claude]

Claude finished @sztomek's task in 2m 1s —— View job

---

Code review (re-run on push — "Address PR comments")

The new commit picks up both of my earlier items, and the result is a tighter fix than the original. Verified against the current tree:

Earlier item	Status
#1 AutoCompleteResult had the identical latent bug	✅ Fixed — data object Unknown + withDefaultValue(Unknown) (AutoCompleteResponse.kt:22,28), autoCompleteSearch switched map → mapNotNull (ImprovedSearchManagerImpl.kt:19,30), with adapter + manager tests
#2 Required non-null fields that no consumer reads	✅ Partly — slug and podcastSlug now default to "" (CombinedSearchResponse.kt:19,36)
Test suggestion: add an episode entry alongside the fallback	✅ Added (CombinedSearchResponseTest.kt:33-46)
Test suggestion I retracted: "no type key" case	✅ Correctly not added — it would still throw Missing label for type

The core reasoning holds: PolymorphicJsonAdapterFactory throws from labelIndex() on an unregistered label, and because that throw happens inside the generated response adapter's list loop, one "type":"network" entry aborts decoding of the whole payload. withDefaultValue installs a fallback that skipValue()s the object and returns Unknown, so decoding continues. Both when blocks stay exhaustive over their sealed types, so a future subtype is a compile error rather than a silent miss.

Findings

1. The new = "" defaults don't cover an explicit null (low)

Detailed inline. Moshi codegen uses a Kotlin default only when the key is absent — "slug": null still throws JsonDataException: Non-null value 'slug' was null at $.results[N] and takes the whole response down, which is the failure mode this PR exists to kill. String? = null closes both cases; String = "" closes one. Same reasoning applies to EpisodeResult.url (CombinedSearchResponse.kt:29), which is still required and, like the slugs, read by no consumer.

Fine as a follow-up — the reported bug is fixed regardless.

2. Dropped results are still invisible (low, unchanged from last review)

Nothing logs a discard in either Unknown branch. If the server broadens network results or adds a type, the client silently returns a shorter list and no signal reaches anyone. A LogBuffer.i in the Unknown branches would make the next occurrence visible. Separate product question: network results are an intentional server feature, so dropping them means podcast networks never surface in search — worth a tracking issue if one doesn't exist.

Smaller notes

• PodcastResult.title is nullable and filtered at ImprovedSearchManagerImpl.kt:40, while EpisodeResult.title is required and hard-fails the response. Asymmetric, though title at least is consumed for episodes.
• Pre-existing, not from this PR: publishedDate: Date = Date() (CombinedSearchResponse.kt:28) silently substitutes "now" for a missing date, so a malformed episode renders with a wrong-but-plausible timestamp rather than being dropped.
• Both new tests assert on it::class.simpleName strings. It reads fine and keeps the expectations short, but assertTrue(results[1] is CombinedResult.Unknown) would survive a rename. Taste, not a defect.
• The mock stub autoCompleteSearch(any(), anyOrNull(), anyOrNull()) matches the service signature (query, termsLimit, podcastsLimit — AutoCompleteSearchService.kt:8-12); anyOrNull() is required for the two nullable Int? params, which is right.
• AutoCompleteResponseTest's hand-built Moshi needs no extra adapters — PodcastResultValue is codegen-generated and resolved reflectively. CombinedSearchResponseTest does need its Rfc3339DateJsonAdapter line, because the polymorphic factory resolves subtype adapters eagerly.
• Test placement (src/test/kotlin in servers, src/test/java in repositories) matches each module's existing layout ✅.
• Informational only: PolymorphicJsonAdapter.toJson throws IllegalArgumentException for a value whose class isn't a registered subtype, so serialising either Unknown would fail. Neither type is ever serialised (both are response-only).
• No security or performance implications — no new I/O, allocation, or parsing on any hot path.

Verification

I still could not run ./gradlew — the command requires approval this job doesn't have, and there's no populated Gradle cache in the runner. Both test tasks in the PR description are unverified by me; reading them, the assertions follow from the code as written. Add Bash(./gradlew*) to --allowedTools if you'd like future reviews to execute the suite. The Moshi claims above are read off the 1.15.2 PolymorphicJsonAdapter / codegen behaviour rather than an executed test.

Verdict: ships. Both items I flagged are addressed, the tests now cover both adapters and both registered subtypes plus the fallback, and the remaining points are follow-up-grade. No changes pushed — review feedback only.

---

• Gather context (PR diff vs origin/main)
• Read changed files + all consumers of CombinedResult / AutoCompleteResult
• Confirm the two earlier findings are addressed
• Check the new defaults' actual Moshi semantics
• Attempt to run tests — blocked, see Verification
• Post review
  • branch fix/tv-search-network-crash

[claude]

Good change — slug and podcastSlug are read by nobody (ImprovedSearchManagerImpl.kt:37-61 ignores both, and there are no other consumers of CombinedResult), so requiring them was pure downside.

One caveat on how far a Kotlin default actually protects you: Moshi's generated adapter uses the default only when the key is absent. If the server sends "slug": null explicitly, codegen still hits Util.unexpectedNull(...) and throws JsonDataException: Non-null value 'slug' was null at $.results[N], which aborts the whole response exactly like the old bug. String? = null covers both the absent and explicit-null cases; String = "" covers only the first. Given the motivation here is "one weird result shouldn't wipe out the search", nullable is the stronger guarantee.

Related, and the same argument as slug: url (line 29) is still non-null-required and is likewise never read by any consumer. title/podcastUuid/podcastTitle are at least consumed, though it's a bit asymmetric that PodcastResult.title is nullable-and-filtered (ImprovedSearchManagerImpl.kt:40) while EpisodeResult.title hard-fails the response.

Not blocking — the PR's stated bug is fixed either way.

Fix this →

[wpmobilebot]

Version 8.19 has now entered code-freeze, so the milestone of this PR has been updated to 8.20.

[claude]

Claude finished @sztomek's task in 1m 58s —— View job

---

Code review (re-run on push — 3bb04b2 "Address PR comments")

The new commit closes the last item I flagged. Verified against the current tree:

Earlier item	Status
= "" defaults don't cover an explicit null	✅ Fixed — slug, podcastSlug, and url are now String? = null (CombinedSearchResponse.kt:19,29,36)
Regression test for explicit null	✅ Added (CombinedSearchResponseTest.kt:48-62)
AutoCompleteResult had the same latent bug	✅ Fixed in the previous commit

Both changes are safe: I grepped every reference to CombinedResult across the repo and slug / podcastSlug / url are read by no consumer — ImprovedSearchManagerImpl.kt:39-57 is the only mapping site and it touches none of them. So widening them to nullable can't break a call site, and it removes three more ways a single malformed result could take down the whole response.

Both when blocks stay exhaustive over their sealed types, so a future subtype is a compile error rather than a silent miss.

Findings

1. Fields nobody reads could just be deleted (low, simplification)

slug, podcastSlug, and url are now nullable-with-default and read by zero consumers. Moshi's generated adapters skipValue() unknown JSON keys by default, so removing the three properties outright is behaviourally identical to keeping them nullable — and it's one fewer thing that can go wrong when the server changes shape. Keeping them is defensible if you expect to surface episode url or the slugs soon; otherwise they're dead weight that still has to be parsed and allocated per result.

Fix this →

2. EpisodeResult.title / podcastUuid / podcastTitle are the last whole-response failure mode (low)

CombinedSearchResponse.kt:26,32,34 are still non-null-required, so one episode result missing (or nulling) podcast_title still throws JsonDataException and wipes the entire search — the exact symptom this PR exists to kill, just with a rarer trigger. Unlike the slugs, these are consumed, so the fix isn't free: making them nullable means either dropping the item in ImprovedSearchManagerImpl (mirroring what PodcastResult.title already does at line 40) or defaulting. Worth noting the current asymmetry — PodcastResult.title is nullable-and-filtered, EpisodeResult.title hard-fails the response. Follow-up material, not a blocker.

3. Dropped results are still invisible (low, unchanged)

Neither Unknown branch (ImprovedSearchManagerImpl.kt:30,59) logs a discard. If the server broadens network results or adds a type, the client silently returns a shorter list and nobody finds out. A LogBuffer.i would make the next occurrence visible. Separate product question: network results are an intentional server feature, so dropping them means podcast networks never surface in search — worth a tracking issue if one doesn't exist.

Smaller notes

• The new test's episode payload omits duration, exercising the 0L default alongside the explicit nulls — nice coverage bonus.
• PodcastResult.author: String? = "" (line 18) is a nullable field with an empty-string default, which the consumer then .orEmpty()s anyway. Pre-existing and harmless, but the default buys nothing.
• Pre-existing: publishedDate: Date = Date() (line 28) substitutes "now" for a missing date, so a malformed episode renders with a wrong-but-plausible timestamp rather than being dropped.
• No security or performance implications — no new I/O, allocation, or parsing on any hot path; the nullable widening removes potential exception throws.

Verification

I still could not run ./gradlew — the command requires approval this job doesn't have. Both test tasks from the PR description are unverified by me. Reading them, the assertions follow from the code: Moshi codegen assigns null directly for a nullable property whose JSON value is explicitly null (no unexpectedNull check is generated), and uses the Kotlin default only when the key is absent — so all three cases in CombinedSearchResponseTest hold. Add Bash(./gradlew*) to --allowedTools if you'd like future reviews to execute the suite.

Verdict: ships. Every item I raised across the three reviews is now addressed; the remaining points are follow-up-grade. No changes pushed — review feedback only.

---

• Gather context (PR diff vs origin/main — note: the checkout is shallow with no merge base, so I reviewed commit-by-commit plus the changed-file list)
• Read changed files + grep all consumers of CombinedResult / AutoCompleteResult
• Confirm the previous findings are addressed
• Check that widening slug/url/podcastSlug to nullable breaks no call site
• Attempt to run tests — blocked, see Verification
• Post review
  • branch fix/tv-search-network-crash