fix(diagnostics): stop the focus warning crashing, and name the endpoints it reports - #203
Conversation
…annot crash FATAL EXCEPTION on the Shield, from the instrumentation itself: DiagnosticsLogRenderer.renderAttributes(SiloLog.kt:81) DiagnosticsFocusLogger.contentEntryFailed(DiagnosticsInstrumentation.kt:289) TvMainShell$moveFocusToContent(TvMainShell.kt:660) renderAttributes throws on an unregistered attribute while strictAttributeRegistry is on, and contentEntryFailed() passes "route" while the FOCUS category registered only target and action. So the warning added in Silo-Server#199 to make silent focus-entry failures visible killed the app the moment one occurred. Release builds set strict to false and drop the attribute silently, which is why it survived review and only appeared on a debug telemetry build. This is live on main via Silo-Server#199 and wants the same one-line fix there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 28709d691a0fd351b645b6431d55b240c5340790)
…ognised A tester's device produced 52 404s in an eight-minute session, all logged as "/api/v1/other" — the largest error signal on the device and completely unactionable, because the normaliser collapses any path whose tail matches no template down to a bare bucket. An allowlisted resource now keeps its name: /api/v1/playback/other rather than /api/v1/other. That resource already appears in every other path logged for it, so nothing new is disclosed. An UNRECOGNISED resource stays anonymous, deliberately. That is a considered privacy decision rather than an oversight — the existing test makes the point with "/api/v1/private/private-id" -> "/api/v1/other" — and it is unchanged here, as is the query-string case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 5ffba5f5630629793db34f63c5c903f2860124d1)
Every diagnostics network log runs its path through `safeDiagnosticsNetworkPath`, which keeps a path only when its resource root has an entry in `API_ROUTE_TEMPLATES` and buckets everything else into `/api/v1/other`. Seven roots the client actually calls were missing: favorites, history, library-playback-prefs, metadata, onboarding, watch and watchlist. Two of them carry the loudest 4xx the client emits. `checkFavorite` and `checkWatchlist` answer "not in the list" with a 404, and the TV detail screen fires one per episode on every season load, so on a live tester's Fire TV those 404s were 116 of the 121 four-hundreds recorded — all of them landing in `/api/v1/other`, where the endpoint cannot be named and the pattern cannot be seen. The N+1 itself is a separate fix; this is about being able to observe it. Derived by diffing the paths the client builds under `shared/.../network` against the template keys, so the gap is closed as a set rather than one report at a time. The stale keys in the other direction (stream, users, watch-together) are left alone — they cost nothing and the routes still exist server-side. Verified: :android-shared:testDebugUnitTest --tests *DiagnosticsInstrumentationTest* passes, including the existing assertions that a query string still collapses the whole path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDiagnostics route sanitization now preserves recognized resource names for unmatched paths, adds allowlisted API templates, registers the ChangesDiagnostics route handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/diagnostics/DiagnosticsInstrumentationTest.kt (1)
46-53: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover every newly allowlisted route template.
This test covers
favorites/{id}andwatchlist/{id}, but it does not cover the newhistory,library-playback-prefs,metadata/ai/status,onboarding, orwatch/{id}mappings. Add focused assertions for their root and dynamic templates.Suggested focused assertions
+ mapOf( + "/api/v1/favorites" to "/api/v1/favorites", + "/api/v1/history" to "/api/v1/history", + "/api/v1/library-playback-prefs" to "/api/v1/library-playback-prefs", + "/api/v1/library-playback-prefs/profile-1" to "/api/v1/library-playback-prefs/{id}", + "/api/v1/metadata/ai/status" to "/api/v1/metadata/ai/status", + "/api/v1/onboarding/flow" to "/api/v1/onboarding/flow", + "/api/v1/onboarding/state" to "/api/v1/onboarding/state", + "/api/v1/onboarding/progress" to "/api/v1/onboarding/progress", + "/api/v1/watch/item-1" to "/api/v1/watch/{id}", + "/api/v1/watchlist" to "/api/v1/watchlist", + ).forEach { (rawPath, expected) -> + assertEquals(expected, safeDiagnosticsNetworkPath(rawPath)) + }As per coding guidelines, add focused tests for shared logic only when behavior is critical or high risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/diagnostics/DiagnosticsInstrumentationTest.kt` around lines 46 - 53, Extend the diagnostics path assertions in DiagnosticsInstrumentationTest around safeDiagnosticsNetworkPath to cover every newly allowlisted route: history, library-playback-prefs, metadata/ai/status, onboarding, and watch/{id}. Add focused checks for each route’s root and dynamic template behavior, matching the existing favorites and watchlist assertion style.Source: Coding guidelines
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/diagnostics/SiloLog.kt (1)
167-171: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a strict-registry regression test for
FOCUS.route.
renderAttributesthrows at Line 80 when an attribute is unregistered or has the wrong kind. This change fixes a debug-build crash, but the supplied diagnostics tests do not render aFOCUSentry withroute. Add a focused test that passesSiloLogAttribute.TextforFOCUS.routewith strict validation enabled and verifies that rendering succeeds.As per coding guidelines, add focused tests for shared logic only when behavior is critical or high risk.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/diagnostics/SiloLog.kt` around lines 167 - 171, Add a focused regression test for the shared SiloLog rendering logic that enables strict attribute validation, renders a FOCUS entry containing SiloLogAttribute.Text for the route attribute, and verifies rendering completes successfully without throwing. Use the existing diagnostics test fixtures and assertion patterns, targeting renderAttributes and the FOCUS route registration.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@android-shared/src/androidMain/kotlin/org/siloserver/silo/common/diagnostics/SiloLog.kt`:
- Around line 167-171: Add a focused regression test for the shared SiloLog
rendering logic that enables strict attribute validation, renders a FOCUS entry
containing SiloLogAttribute.Text for the route attribute, and verifies rendering
completes successfully without throwing. Use the existing diagnostics test
fixtures and assertion patterns, targeting renderAttributes and the FOCUS route
registration.
In
`@android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/diagnostics/DiagnosticsInstrumentationTest.kt`:
- Around line 46-53: Extend the diagnostics path assertions in
DiagnosticsInstrumentationTest around safeDiagnosticsNetworkPath to cover every
newly allowlisted route: history, library-playback-prefs, metadata/ai/status,
onboarding, and watch/{id}. Add focused checks for each route’s root and dynamic
template behavior, matching the existing favorites and watchlist assertion
style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4469db92-1e37-4e3c-8257-93f44af910d5
📒 Files selected for processing (3)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/diagnostics/DiagnosticsInstrumentation.ktandroid-shared/src/androidMain/kotlin/org/siloserver/silo/common/diagnostics/SiloLog.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/diagnostics/DiagnosticsInstrumentationTest.kt
…attribute Two reviewers landed on the same gap independently, so it was worth closing. The route-template test asserted two of the seven roots this branch added. 116 of 121 recorded 4xx were unnameable precisely because these collapsed to "/api/v1/other", and a template that silently stops matching puts them back there with nothing failing. Every root and dynamic form is now asserted, plus the other half of the bargain: an allowlisted resource names itself on an unmatched tail while an unknown resource stays anonymous. Mutation-checked by dropping a template. The FOCUS.route registration had no strict-renderer test, which is what let Silo-Server#199 ship the crash in the first place: an unregistered attribute throws while strictAttributeRegistry is on, so the warning meant to surface silent focus failures killed debug builds the moment it fired. A strict render of exactly that shape now guards it, with a companion asserting a wrong-KIND route is still rejected. android-shared 1101 -> 1105, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
1. #199's new warning crashes debug builds the moment it fires
#199 added
DiagnosticsFocusLogger.contentEntryFailed(route), which reports arouteattribute in theFOCUScategory. TheFOCUSregistry inSiloLog.ktdeclares onlytargetandaction, and an unregistered attribute hits:So in debug builds the warning added to make silent focus failures visible throws the instant it fires — and it fires exactly when TV content focus entry fails, which is the condition #199 exists to diagnose. In release builds it does not crash, but
routeis dropped, shipping the warning without the one field that makes it actionable.Fix: register
route.2. An allowlisted resource now keeps its name
safeDiagnosticsNetworkPathcollapsed any path whose tail matched no template down to a bare/api/v1/other. A tester's device produced 52 404s in an eight-minute session, all logged that way — the largest error signal on the device, completely unactionable.An allowlisted resource now keeps its name:
/api/v1/playback/otherrather than/api/v1/other. That resource already appears in every other path logged for it, so nothing new is disclosed. An unrecognised resource stays anonymous, deliberately — the existing test makes the point with/api/v1/private/private-id→/api/v1/other, and that is unchanged, as is the query-string case.3. Seven API roots had no route template at all
favorites,history,library-playback-prefs,metadata,onboarding,watch,watchlist— all called by the client, none inAPI_ROUTE_TEMPLATES, so all bucketed.Two of them carry the loudest 4xx the client emits:
checkFavoriteandcheckWatchlistanswer "not in the list" with a 404, and the TV detail screen fires one per episode on every season load. On a live device those 404s were 116 of the 121 four-hundreds recorded. (The N+1 itself is a separate fix; this is about being able to see it.)Derived by diffing the paths the client builds under
shared/.../networkagainst the template keys, so the gap is closed as a set rather than one report at a time. Stale keys in the other direction (stream,users,watch-together) are left alone — they cost nothing and the routes still exist server-side.Verification
:android-shared:testDebugUnitTest— 1101 tests, 0 failures.Summary by CodeRabbit