fix(connectivity): drive reachability from real repository traffic
🏗️ Build and Test JellyTau / Run Tests (push) Successful in 3m32s
Traceability Validation / Check Requirement Traces (push) Successful in 20s
🏗️ Build and Test JellyTau / Build Android APK (push) Successful in 18m27s
Build & Release / Run Tests (push) Successful in 3m36s
Build & Release / Build Linux (push) Successful in 15m43s
Build & Release / Build Android (push) Successful in 18m40s
Build & Release / Create Release (push) Failing after 22s
🏗️ Build and Test JellyTau / Run Tests (push) Successful in 3m32s
Traceability Validation / Check Requirement Traces (push) Successful in 20s
🏗️ Build and Test JellyTau / Build Android APK (push) Successful in 18m27s
Build & Release / Run Tests (push) Successful in 3m36s
Build & Release / Build Linux (push) Successful in 15m43s
Build & Release / Build Android (push) Successful in 18m40s
Build & Release / Create Release (push) Failing after 22s
The offline/online switch was janky because two independent systems decided "online" and never communicated: - ConnectivityMonitor owned is_server_reachable (drove the UI banner) but learned reachability only from a standalone /System/Info/Public ping loop and from auth/login calls. - HybridRepository served all real data by racing cache-vs-server but never read or wrote reachability. So the banner reflected a side-channel poller, not the system the user actually experienced: a successful ping could read "online" while authenticated data calls 401'd or timed out, and three different timeout regimes (5s ping / 30s data / 100ms cache race) flapped against each other. Unify into a single source of truth: - Extract a cheap, cloneable ConnectivityReporter that owns all reachability transitions and event emission. - OnlineRepository reports the outcome of every server request to the reporter, classified via RepoError: Ok/Authentication/NotFound/Server => reachable (the server answered), Network => offline candidate, Database/Offline => ignored (not a server signal). - Time-window debounce (OFFLINE_CONFIRM_WINDOW = 5s): flip offline only after sustained network failure; recover instantly on the first success. - Demote the ping loop to an offline-only recovery probe (no online polling; real traffic is the signal when online). - Frontend: navigator.onLine is now advisory (triggers a recheck instead of forcing offline); removed the dead markReachable/markUnreachable store methods. Docs updated (README, 07-connectivity, 03-data-flow, 02-svelte-frontend) to describe the new model and fix pre-existing drift (HTTP client is 30s timeout + 5s ping, not the documented 10s/base_url). Tests: 12 connectivity tests (debounce, instant recovery, RepoError classification through report_outcome). Full suite: 398 Rust + 384 frontend passing, svelte-check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -7,6 +7,7 @@ use log::{debug, error, info};
|
||||
use log::warn;
|
||||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use crate::connectivity::ConnectivityReporter;
|
||||
use crate::jellyfin::HttpClient;
|
||||
use super::{MediaRepository, types::*};
|
||||
|
||||
@@ -16,6 +17,10 @@ pub struct OnlineRepository {
|
||||
server_url: String,
|
||||
user_id: String,
|
||||
access_token: String,
|
||||
/// Reports the outcome of every server request to the connectivity monitor.
|
||||
/// This is the source of truth for the offline/online banner. `None` in
|
||||
/// tests / contexts where connectivity tracking isn't wired up.
|
||||
connectivity: Option<ConnectivityReporter>,
|
||||
}
|
||||
|
||||
impl OnlineRepository {
|
||||
@@ -30,6 +35,44 @@ impl OnlineRepository {
|
||||
server_url,
|
||||
user_id,
|
||||
access_token,
|
||||
connectivity: None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Attach a connectivity reporter so server outcomes drive the reachability
|
||||
/// state observed by the UI. See `report_outcome`.
|
||||
pub fn with_connectivity(mut self, reporter: ConnectivityReporter) -> Self {
|
||||
self.connectivity = Some(reporter);
|
||||
self
|
||||
}
|
||||
|
||||
/// Feed a request outcome into the connectivity monitor.
|
||||
///
|
||||
/// Classification (matches docs/architecture/07-connectivity.md):
|
||||
/// - `Ok` / `Authentication` / `NotFound` / `Server` → the server answered,
|
||||
/// so it is reachable → `report_success` (instant recovery).
|
||||
/// - `Network` → connection-level failure → `report_network_failure`
|
||||
/// (subject to the time-window debounce before going offline).
|
||||
/// - `Database` → not a server signal → ignored.
|
||||
async fn report_outcome<T>(&self, result: &Result<T, RepoError>) {
|
||||
let Some(reporter) = &self.connectivity else {
|
||||
return;
|
||||
};
|
||||
|
||||
match result {
|
||||
Ok(_)
|
||||
| Err(RepoError::Authentication { .. })
|
||||
| Err(RepoError::NotFound { .. })
|
||||
| Err(RepoError::Server { .. }) => {
|
||||
reporter.report_success().await;
|
||||
}
|
||||
Err(RepoError::Network { message }) => {
|
||||
reporter.report_network_failure(Some(message.clone())).await;
|
||||
}
|
||||
Err(RepoError::Database { .. }) | Err(RepoError::Offline) => {
|
||||
// Local-side errors (cache failure / already-offline) — not a
|
||||
// statement about the server's reachability, so ignore them.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -63,6 +106,12 @@ impl OnlineRepository {
|
||||
|
||||
/// Make authenticated GET request
|
||||
async fn get_json<T: for<'de> Deserialize<'de>>(&self, endpoint: &str) -> Result<T, RepoError> {
|
||||
let result = self.get_json_inner(endpoint).await;
|
||||
self.report_outcome(&result).await;
|
||||
result
|
||||
}
|
||||
|
||||
async fn get_json_inner<T: for<'de> Deserialize<'de>>(&self, endpoint: &str) -> Result<T, RepoError> {
|
||||
let url = format!("{}{}", self.server_url, endpoint);
|
||||
|
||||
let request = self.http_client.client.get(&url)
|
||||
@@ -110,6 +159,12 @@ impl OnlineRepository {
|
||||
|
||||
/// Make authenticated POST request
|
||||
async fn post_json<T: Serialize>(&self, endpoint: &str, body: &T) -> Result<(), RepoError> {
|
||||
let result = self.post_json_inner(endpoint, body).await;
|
||||
self.report_outcome(&result).await;
|
||||
result
|
||||
}
|
||||
|
||||
async fn post_json_inner<T: Serialize>(&self, endpoint: &str, body: &T) -> Result<(), RepoError> {
|
||||
let url = format!("{}{}", self.server_url, endpoint);
|
||||
|
||||
let request = self.http_client.client.post(&url)
|
||||
@@ -145,6 +200,16 @@ impl OnlineRepository {
|
||||
&self,
|
||||
endpoint: &str,
|
||||
body: &T,
|
||||
) -> Result<R, RepoError> {
|
||||
let result = self.post_json_response_inner(endpoint, body).await;
|
||||
self.report_outcome(&result).await;
|
||||
result
|
||||
}
|
||||
|
||||
async fn post_json_response_inner<T: Serialize, R: for<'de> Deserialize<'de>>(
|
||||
&self,
|
||||
endpoint: &str,
|
||||
body: &T,
|
||||
) -> Result<R, RepoError> {
|
||||
let url = format!("{}{}", self.server_url, endpoint);
|
||||
|
||||
@@ -1156,23 +1221,29 @@ impl MediaRepository for OnlineRepository {
|
||||
let endpoint = format!("/Users/{}/FavoriteItems/{}", self.user_id, item_id);
|
||||
let url = format!("{}{}", self.server_url, endpoint);
|
||||
|
||||
let request = self.http_client.client.delete(&url)
|
||||
.header("X-Emby-Authorization", self.auth_header())
|
||||
.build()
|
||||
.map_err(|e| RepoError::Network {
|
||||
message: format!("Failed to build request: {}", e),
|
||||
})?;
|
||||
let result = async {
|
||||
let request = self.http_client.client.delete(&url)
|
||||
.header("X-Emby-Authorization", self.auth_header())
|
||||
.build()
|
||||
.map_err(|e| RepoError::Network {
|
||||
message: format!("Failed to build request: {}", e),
|
||||
})?;
|
||||
|
||||
let response = self.http_client.request_with_retry(request).await
|
||||
.map_err(|e| RepoError::Network { message: e.to_string() })?;
|
||||
let response = self.http_client.request_with_retry(request).await
|
||||
.map_err(|e| RepoError::Network { message: e.to_string() })?;
|
||||
|
||||
if !response.status().is_success() {
|
||||
return Err(RepoError::Server {
|
||||
message: format!("HTTP {}", response.status()),
|
||||
});
|
||||
if !response.status().is_success() {
|
||||
return Err(RepoError::Server {
|
||||
message: format!("HTTP {}", response.status()),
|
||||
});
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
.await;
|
||||
|
||||
Ok(())
|
||||
self.report_outcome(&result).await;
|
||||
result
|
||||
}
|
||||
|
||||
async fn get_person(&self, person_id: &str) -> Result<MediaItem, RepoError> {
|
||||
@@ -1397,6 +1468,91 @@ mod tests {
|
||||
)
|
||||
}
|
||||
|
||||
/// Build a repository wired to a real ConnectivityReporter so we can assert
|
||||
/// how `report_outcome` classifies each `RepoError` into reachability.
|
||||
/// (No app handle → event emission is a harmless no-op.)
|
||||
fn create_test_repository_with_connectivity(
|
||||
) -> (OnlineRepository, crate::connectivity::ConnectivityReporter) {
|
||||
let monitor_http = HttpClient::new(crate::jellyfin::HttpConfig::default())
|
||||
.expect("Failed to create HTTP client for monitor");
|
||||
let monitor = crate::connectivity::ConnectivityMonitor::new(monitor_http);
|
||||
let reporter = monitor.reporter();
|
||||
let repo = create_test_repository().with_connectivity(reporter.clone());
|
||||
(repo, reporter)
|
||||
}
|
||||
|
||||
/// `report_outcome` is the seam between repository traffic and the
|
||||
/// connectivity monitor. Verify each `RepoError` variant routes correctly:
|
||||
/// - the server answering at all (Ok / 401 / 404 / 5xx) ⇒ reachable
|
||||
/// - a network-level failure ⇒ marked unreachable (debounce reduced for test)
|
||||
/// - local-side errors (Database / Offline) ⇒ no effect on reachability
|
||||
///
|
||||
/// @req-test: UR-002 - Access media when online or offline
|
||||
/// @req-test: DR-013 - Repository pattern for online/offline data access
|
||||
#[tokio::test]
|
||||
async fn test_report_outcome_classifies_server_answered_as_reachable() {
|
||||
let (repo, reporter) = create_test_repository_with_connectivity();
|
||||
|
||||
// Drive offline first so we can observe "recover to reachable".
|
||||
for err in [
|
||||
RepoError::Authentication { message: "401".into() },
|
||||
RepoError::NotFound { message: "404".into() },
|
||||
RepoError::Server { message: "500".into() },
|
||||
] {
|
||||
reporter.mark_unreachable_for_test().await;
|
||||
assert!(!reporter.is_reachable().await, "precondition: offline");
|
||||
|
||||
let result: Result<(), RepoError> = Err(err);
|
||||
repo.report_outcome(&result).await;
|
||||
|
||||
assert!(
|
||||
reporter.is_reachable().await,
|
||||
"a server that answers should be reported reachable"
|
||||
);
|
||||
}
|
||||
|
||||
// Ok should also report reachable.
|
||||
reporter.mark_unreachable_for_test().await;
|
||||
let ok: Result<(), RepoError> = Ok(());
|
||||
repo.report_outcome(&ok).await;
|
||||
assert!(reporter.is_reachable().await, "Ok ⇒ reachable");
|
||||
}
|
||||
|
||||
/// Local-side errors must NOT flip reachability — they say nothing about the
|
||||
/// server.
|
||||
#[tokio::test]
|
||||
async fn test_report_outcome_ignores_local_errors() {
|
||||
let (repo, reporter) = create_test_repository_with_connectivity();
|
||||
|
||||
// Force offline, then a Database/Offline error must leave it offline
|
||||
// (not falsely report reachable).
|
||||
reporter.mark_unreachable_for_test().await;
|
||||
for err in [RepoError::Database { message: "cache".into() }, RepoError::Offline] {
|
||||
let result: Result<(), RepoError> = Err(err);
|
||||
repo.report_outcome(&result).await;
|
||||
assert!(
|
||||
!reporter.is_reachable().await,
|
||||
"local-side error must not change reachability"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// A network error routes through the debounced path. A single failure stays
|
||||
/// online (debounce window not yet elapsed).
|
||||
#[tokio::test]
|
||||
async fn test_report_outcome_network_error_is_debounced() {
|
||||
let (repo, reporter) = create_test_repository_with_connectivity();
|
||||
assert!(reporter.is_reachable().await, "starts online");
|
||||
|
||||
let result: Result<(), RepoError> = Err(RepoError::Network { message: "timeout".into() });
|
||||
repo.report_outcome(&result).await;
|
||||
|
||||
assert!(
|
||||
reporter.is_reachable().await,
|
||||
"a single network failure stays online (debounced)"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_get_audio_stream_url_formats_correctly() {
|
||||
let repo = create_test_repository();
|
||||
|
||||
Reference in New Issue
Block a user