Fix "Unknown request ID" on startup

This was occurring when the selected request before a reload was unsuccessful, because unsuccessful requests aren't persisted to the DB. Instead of showing an error, we now fall back to selecting the most recent request for the recipe.

Closes #238

CHANGELOG.md

@@ -24,6 +24,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - When a modal/dialog is open `q` now exits the dialog instead of the entire app
 - Upgrade to Rust 1.76
 
+### Fixed
+
+- Fix "Unknown request ID" error showing on startup [#238](https://github.com/LucasPickering/slumber/issues/238)
+
 ## [1.3.2] - 2024-05-27
 
 ### Changed

src/tui/view/component/root.rs

@@ -74,19 +74,34 @@ impl Root {
         request_id: Option<RequestId>,
     ) -> anyhow::Result<()> {
         let primary_view = self.primary_view.data();
-        **self.selected_request = if let Some(request_id) = request_id {
-            // Make sure the given ID is valid, and the request is loaded
-            self.request_store.load(request_id)?;
-            Some(request_id)
-        } else if let Some(recipe_id) = primary_view.selected_recipe_id() {
-            // Find the most recent request by recipe+profile
-            let profile_id = primary_view.selected_profile_id();
-            self.request_store
-                .load_latest(profile_id, recipe_id)?
-                .map(RequestState::id)
-        } else {
-            None
+
+        let mut get_id = || -> anyhow::Result<Option<RequestId>> {
+            if let Some(request_id) = request_id {
+                // Make sure the given ID is valid, and the request is loaded.
+                // This will return `None` if the ID wasn't in the DB, which
+                // probably means we had a loading/failed request selected
+                // before reload, so it's gone. Fall back to the top of the list
+                if let Some(request_state) =
+                    self.request_store.load(request_id)?
+                {
+                    return Ok(Some(request_state.id()));
+                }
+            }
+
+            // We don't have a valid persisted ID, find the most recent for the
+            // current recipe+profile
+            if let Some(recipe_id) = primary_view.selected_recipe_id() {
+                let profile_id = primary_view.selected_profile_id();
+                Ok(self
+                    .request_store
+                    .load_latest(profile_id, recipe_id)?
+                    .map(RequestState::id))
+            } else {
+                Ok(None)
+            }
         };
+
+        **self.selected_request = get_id()?;
         Ok(())
     }
 
@@ -276,7 +291,7 @@ mod tests {
     /// Test that, on first render, if there's a persisted request ID, we load
     /// up to that instead of selecting the first in the list
     #[rstest]
-    fn test_load_persistent_request(harness: TestHarness) {
+    fn test_load_persisted_request(harness: TestHarness) {
         let collection = Collection::factory(());
         let recipe_id = collection.first_recipe_id();
         let profile_id = collection.first_profile_id();
@@ -311,6 +326,34 @@ mod tests {
         );
     }
 
+    /// Test that if the persisted request ID isn't in the DB, we'll fall back
+    /// to selecting the most recent request
+    #[rstest]
+    fn test_persisted_request_missing(harness: TestHarness) {
+        let collection = Collection::factory(());
+        let recipe_id = collection.first_recipe_id();
+        let profile_id = collection.first_profile_id();
+        let old_exchange =
+            Exchange::factory((Some(profile_id.clone()), recipe_id.clone()));
+        let new_exchange =
+            Exchange::factory((Some(profile_id.clone()), recipe_id.clone()));
+        harness.database.insert_exchange(&old_exchange).unwrap();
+        harness.database.insert_exchange(&new_exchange).unwrap();
+        harness
+            .database
+            .set_ui(PersistentKey::RequestId, RequestId::new())
+            .unwrap();
+
+        let component = TestComponent::new(harness, Root::new(&collection), ());
+
+        assert_eq!(
+            component.data().selected_request(),
+            Some(&RequestState::Response {
+                exchange: new_exchange
+            })
+        );
+    }
+
     #[rstest]
     fn test_edit_collection(harness: TestHarness) {
         let collection = Collection::factory(());

src/tui/view/state/request_store.rs

@@ -5,7 +5,6 @@ use crate::{
         context::ViewContext, state::RequestStateSummary, RequestState,
     },
 };
-use anyhow::anyhow;
 use itertools::Itertools;
 use std::collections::{hash_map::Entry, HashMap};
 
@@ -39,17 +38,23 @@ impl RequestStore {
     }
 
     /// Load a request from the database by ID. If already present in the store,
-    /// do *not* update it. Only go to the DB if it's missing.
-    pub fn load(&mut self, id: RequestId) -> anyhow::Result<()> {
-        if let Entry::Vacant(entry) = self.requests.entry(id) {
-            let exchange = ViewContext::with_database(|database| {
-                database
-                    .get_request(id)?
-                    .ok_or_else(|| anyhow!("Unknown request ID `{id}`"))
-            })?;
-            entry.insert(RequestState::response(exchange));
-        }
-        Ok(())
+    /// do *not* update it. Only go to the DB if it's missing. Return the loaded
+    /// request. Return `None` only if the ID is not present in the store *or*
+    /// the DB.
+    pub fn load(
+        &mut self,
+        id: RequestId,
+    ) -> anyhow::Result<Option<&RequestState>> {
+        let request = match self.requests.entry(id) {
+            Entry::Occupied(entry) => Some(entry.into_mut()),
+            Entry::Vacant(entry) => {
+                ViewContext::with_database(|database| database.get_request(id))?
+                    .map(|exchange| {
+                        entry.insert(RequestState::response(exchange))
+                    })
+            }
+        };
+        Ok(request.map(|r| &*r))
     }
 
     /// Get the latest request for a specific profile+recipe combo
@@ -117,9 +122,10 @@ mod tests {
     use super::*;
     use crate::{
         http::{Exchange, RequestBuildError, RequestError, RequestRecord},
-        test_util::{assert_err, assert_matches, Factory},
+        test_util::{assert_matches, Factory},
         tui::test_util::{harness, TestHarness},
     };
+    use anyhow::anyhow;
     use chrono::Utc;
     use rstest::rstest;
     use std::sync::Arc;
@@ -197,22 +203,28 @@ mod tests {
             store.get(present_id),
             Some(RequestState::Response { .. })
         );
-        store.load(present_id).expect("Expected success");
+        assert_matches!(
+            store.load(present_id),
+            Ok(Some(RequestState::Response { .. }))
+        );
         assert_matches!(
             store.get(present_id),
             Some(RequestState::Response { .. })
         );
 
         // Not in store, fetch successfully
         assert!(store.get(missing_id).is_none());
-        store.load(missing_id).expect("Expected success");
+        assert_matches!(
+            store.load(missing_id),
+            Ok(Some(RequestState::Response { .. }))
+        );
         assert_matches!(
             store.get(missing_id),
             Some(RequestState::Response { .. })
         );
 
-        // Not in store and not in DB, return error
-        assert_err!(store.load(RequestId::new()), "Unknown request ID");
+        // Not in store and not in DB, return None
+        assert_matches!(store.load(RequestId::new()), Ok(None));
     }
 
     #[rstest]