Correctly handle spaces in paths and show error when diff fails

Cargo.lock

@@ -4463,6 +4463,7 @@ dependencies = [
  "log",
  "octocrab",
  "octocrab-wasm",
+ "percent-encoding",
  "re_ui",
  "reqwest",
  "serde",

Cargo.toml

@@ -33,6 +33,7 @@ image = "0.25.8"
 log = "0.4.28"
 octocrab = { version = "0.49.7", default-features = false, features = ["stream", "jwt-rust-crypto"] }
 octocrab-wasm = { path = "crates/octocrab-wasm" }
+percent-encoding = "2.3.2"
 re_ui = { git = "https://github.com/rerun-io/rerun", branch = "main" }
 reqwest = { version = "0.13.2", default-features = false, features = [] }
 serde = "1.0"

src/diff_image_loader.rs

@@ -72,14 +72,13 @@ impl DiffImageLoader {
         }
     }
 
-    pub fn diff_info(&self, uri: &str) -> Option<DiffInfo> {
-        if let Some(image) = self.diffs.lock().get(uri) {
-            match image {
-                Ok(Poll::Ready(result)) => Some(result.clone()),
-                _ => None,
-            }
-        } else {
-            None
+    /// `Some(Ok(info))` once the diff is ready, `None` while pending or before it has
+    /// started, and `Some(Err(msg))` if the diff failed to load (e.g. mismatched image sizes).
+    pub fn diff_info(&self, uri: &str) -> Option<Result<DiffInfo, String>> {
+        match self.diffs.lock().get(uri) {
+            Some(Ok(Poll::Ready(info))) => Some(Ok(info.clone())),
+            Some(Ok(Poll::Pending)) | None => None,
+            Some(Err(err)) => Some(Err(err.to_string())),
         }
     }
 }
@@ -122,17 +121,17 @@ impl ImageLoader for DiffImageLoader {
                 std::thread::Builder::new()
                     .name(format!("diff for {uri}"))
                     .spawn(move || {
-                        ctx.request_repaint();
                         let result = load_diffs(&ctx, &old_image, &new_image, size_hint, &diff_uri);
                         cache.lock().insert(uri, result.map(Poll::Ready));
+                        ctx.request_repaint();
                     })
                     .expect("Failed to spawn diff thread");
                 #[cfg(target_arch = "wasm32")]
                 {
                     wasm_bindgen_futures::spawn_local(async move {
-                        ctx.request_repaint();
                         let result = load_diffs(&ctx, &old_image, &new_image, size_hint, &diff_uri);
                         cache.lock().insert(uri, result.map(Poll::Ready));
+                        ctx.request_repaint();
                     });
                 }
             }

src/loaders/pr_loader.rs

@@ -9,9 +9,23 @@ use egui_inbox::{UiInbox, UiInboxSender};
 use futures::{StreamExt as _, TryStreamExt as _};
 use octocrab::models::repos::DiffEntryStatus;
 use octocrab::{Octocrab, Result};
+use percent_encoding::{AsciiSet, CONTROLS, utf8_percent_encode};
 use std::pin::pin;
 use std::task::Poll;
 
+/// Percent-encode path segments for use in a URL path, preserving `/`.
+/// Mirrors the `PATH` set from the URL spec but lets `utf8_percent_encode` handle non-ASCII.
+const PATH_SEGMENT: &AsciiSet = &CONTROLS
+    .add(b' ')
+    .add(b'"')
+    .add(b'#')
+    .add(b'<')
+    .add(b'>')
+    .add(b'?')
+    .add(b'`')
+    .add(b'{')
+    .add(b'}');
+
 type Sender = UiInboxSender<Option<Result<Snapshot>>>;
 
 pub struct PrLoader {
@@ -114,18 +128,23 @@ async fn resolve_url(
     file_path: &str,
     logged_in: bool,
 ) -> Option<String> {
+    let encoded_path = utf8_percent_encode(file_path, PATH_SEGMENT).to_string();
     if logged_in {
         let content = repo_client
             .repos()
             .get_content()
-            .path(file_path)
+            .path(&encoded_path)
             .r#ref(commit_sha)
             .send()
             .await
             .ok()?;
         content.items.first()?.download_url.clone()
     } else {
-        Some(create_media_url(repo_client.repo(), commit_sha, file_path))
+        Some(create_media_url(
+            repo_client.repo(),
+            commit_sha,
+            &encoded_path,
+        ))
     }
 }
 

src/viewer/diff_view.rs

@@ -10,21 +10,33 @@ pub fn diff_view(ui: &mut Ui, state: &ViewerAppStateRef<'_>) {
             state.app.settings.options,
         );
 
-        if let Some(info) =
-            diff_uri.and_then(|diff_uri| state.app.diff_image_loader.diff_info(&diff_uri))
-        {
-            if info.diff == 0 {
-                ui.strong("All differences below threshold!");
-            } else {
+        let diff_info = diff_uri
+            .as_deref()
+            .and_then(|uri| state.app.diff_image_loader.diff_info(uri));
+
+        match &diff_info {
+            Some(Ok(info)) => {
+                if info.diff == 0 {
+                    ui.strong("All differences below threshold!");
+                } else {
+                    ui.label(
+                        RichText::new(format!("Diff pixels: {}", info.diff))
+                            .color(ui.visuals().warn_fg_color),
+                    );
+                }
+            }
+            Some(Err(err)) => {
                 ui.label(
-                    RichText::new(format!("Diff pixels: {}", info.diff))
-                        .color(ui.visuals().warn_fg_color),
+                    RichText::new(format!("Diff failed: {err}")).color(ui.visuals().error_fg_color),
                 );
             }
-        } else {
-            ui.label("No diff info yet...");
+            None => {
+                ui.label("No diff info yet...");
+            }
         }
 
+        let diff_failed = matches!(diff_info, Some(Err(_)));
+
         let rect = ui.available_rect_before_wrap();
 
         let old = snapshot.old_image(state.app);
@@ -51,7 +63,9 @@ pub fn diff_view(ui: &mut Ui, state: &ViewerAppStateRef<'_>) {
             ui.place(rect, new);
         }
 
-        if let Some(diff) = diff {
+        if let Some(diff) = diff
+            && !diff_failed
+        {
             ui.place(rect, diff);
         }