Fix null pointer safety in BookDetails

- Remove raw Book pointer storage, use book_id for lookups instead
- Look up fresh book data via BookList::findById() for all operations
- Subscribe to BookList signals (removed, updated, reset) to track changes
- Emit signalBookRemoved when displayed book is deleted
- Navigate back to shelf automatically when book is removed
- Add clear() method and proper destructor with signal disconnection
- Add refresh_display() for safe re-rendering from current state

This prevents crashes when books are removed while viewing details.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

PLAN.md

@@ -28,20 +28,17 @@ Remaining (moved to Phase 2):
 - [ ] Add visual feedback (toast/notification) for tag operations
 
 ### 1.2 Fix Null Pointer Safety in BookDetails
-**Files**: `src/BookDetails.cpp`
-**Status**: Not Started
-**Location**: `src/BookDetails.cpp:57-89`
-
-Current behavior:
-- `on_open_button_clicked()` already has `if (m_book)` check (line 58)
-- `on_add_tag_button_clicked()` already has `if (m_book)` check (line 81)
-- BUT: `m_book` pointer could become dangling if book is deleted from list
+**Files**: `src/BookDetails.cpp`, `src/BookDetails.hpp`, `src/BibliothecaWindow.cpp`
+**Status**: COMPLETED
 
-Tasks:
-- [ ] Handle case where book is deleted while details view is open
-- [ ] Subscribe to BookList signals to detect book removal
-- [ ] Clear details view or navigate back when displayed book is removed
-- [ ] Consider storing book ID instead of pointer and looking up fresh data
+Implemented:
+- [x] Removed raw Book pointer storage - now only stores `m_book_id`
+- [x] All operations look up fresh book data via `m_book_list.findById()`
+- [x] Subscribe to BookList signals (signalBookRemoved, signalBookUpdated, signalReset)
+- [x] Emit `signalBookRemoved()` when displayed book is deleted
+- [x] BibliothecaWindow navigates back to shelf when book is removed
+- [x] Added `clear()` method and proper destructor with signal disconnection
+- [x] `refresh_display()` method for safe re-rendering from current data
 
 ### 1.3 Fix Const-Correctness in DatabaseManager
 **Files**: `src/DatabaseManager.cpp`, `src/DatabaseManager.hpp`
@@ -58,20 +55,17 @@ Tasks:
 - [ ] Document thread-safety guarantees in header comments
 
 ### 1.4 Fix Inefficient Tag Updates
-**Files**: `src/BookDetails.cpp`, `src/BookList.cpp`
-**Status**: Not Started
-**Location**: `src/BookDetails.cpp:85`
+**Files**: `src/BookDetails.cpp`
+**Status**: COMPLETED
 
-Current behavior:
-- `on_add_tag_button_clicked()` calls `m_book_list.loadAll()` after adding tag
-- This reloads ALL books from database, losing selection/scroll state
-- Very inefficient for large libraries
+Previous behavior:
+- `on_add_tag_button_clicked()` called `m_book_list.loadAll()` after adding tag
+- This reloaded ALL books from database, losing selection/scroll state
 
-Tasks:
-- [ ] Add `BookList::updateBookTags(book_id)` method for incremental update
-- [ ] Or: Directly update the Book object's tags and emit `signalBookUpdated`
-- [ ] Refresh only the displayed BookDetails, not entire list
-- [ ] Preserve scroll position and selection state in BookShelf
+Implemented:
+- [x] `refresh_tags()` now only queries tags for the specific book
+- [x] No full list reload - only the tag FlowBox is refreshed
+- [x] Selection and scroll state in BookShelf are preserved
 
 ---
 

src/BibliothecaWindow.cpp

@@ -96,6 +96,10 @@ BibliothecaWindow::BibliothecaWindow(DatabaseManager& db, BookList& bookList)
   m_stack.add(m_placeholder, "empty");
   m_stack.add(*m_shelf, "shelf");
   m_bookDetails = std::make_unique<BookDetails>(m_db, m_bookList);
+  m_bookDetails->signalBookRemoved().connect([this]() {
+    // Navigate back to shelf when displayed book is removed
+    m_stack.set_visible_child("shelf");
+  });
   m_stack.add(*m_bookDetails, "details");
   m_stack.add(m_noResults, "noresults");
 

src/BookDetails.cpp

@@ -42,6 +42,24 @@ BookDetails::BookDetails(DatabaseManager& db, BookList& book_list) :
   m_add_tag_button.signal_clicked().connect(sigc::mem_fun(*this, &BookDetails::on_add_tag_button_clicked));
   tag_entry_box.append(m_add_tag_button);
   append(tag_entry_box);
+
+  // Connect to BookList signals to handle book removal/updates
+  m_conn_removed = m_book_list.signalBookRemoved().connect(
+    sigc::mem_fun(*this, &BookDetails::on_book_removed));
+  m_conn_updated = m_book_list.signalBookUpdated().connect(
+    [this](const Book& book) {
+      if (book.id() == m_book_id) {
+        refresh_display();
+      }
+    });
+  m_conn_reset = m_book_list.signalReset().connect(
+    sigc::mem_fun(*this, &BookDetails::on_book_list_changed));
+}
+
+BookDetails::~BookDetails() {
+  m_conn_removed.disconnect();
+  m_conn_updated.disconnect();
+  m_conn_reset.disconnect();
 }
 
 Gtk::Widget* BookDetails::create_tag_chip(const std::string& tag_name) {
@@ -74,6 +92,8 @@ void BookDetails::refresh_tags() {
     m_tags_box.remove(*child);
   }
 
+  if (m_book_id.empty()) return;
+
   // Reload tags from database
   auto tags = m_db.get_tags_for_book(m_book_id);
   for (const auto& tag : tags) {
@@ -81,39 +101,92 @@ void BookDetails::refresh_tags() {
   }
 }
 
+void BookDetails::refresh_display() {
+  if (m_book_id.empty()) return;
+
+  auto book_opt = m_book_list.findById(m_book_id);
+  if (!book_opt) {
+    // Book no longer exists
+    clear();
+    m_signalBookRemoved.emit();
+    return;
+  }
+
+  const auto& book = *book_opt;
+  m_cover.set(book.cover());
+  m_title.set_markup("<span weight='bold' size='large'>" + Glib::Markup::escape_text(book.title()) + "</span>");
+  m_author.set_markup("<span size='medium'>" + Glib::Markup::escape_text(book.author()) + "</span>");
+  refresh_tags();
+}
+
 void BookDetails::set_book(const Book* book) {
-  m_book = book;
-  if (m_book) {
-    m_book_id = m_book->id();
-    m_cover.set(m_book->cover());
-    m_title.set_markup("<span weight='bold' size='large'>" + Glib::Markup::escape_text(m_book->title()) + "</span>");
-    m_author.set_markup("<span size='medium'>" + Glib::Markup::escape_text(m_book->author()) + "</span>");
+  if (book) {
+    m_book_id = book->id();
+    m_cover.set(book->cover());
+    m_title.set_markup("<span weight='bold' size='large'>" + Glib::Markup::escape_text(book->title()) + "</span>");
+    m_author.set_markup("<span size='medium'>" + Glib::Markup::escape_text(book->author()) + "</span>");
     refresh_tags();
   } else {
-    m_book_id.clear();
+    clear();
+  }
+}
+
+void BookDetails::clear() {
+  m_book_id.clear();
+  m_cover.clear();
+  m_title.set_markup("<span weight='bold' size='large'></span>");
+  m_author.set_markup("<span size='medium'></span>");
+  while (auto child = m_tags_box.get_child_at_index(0)) {
+    m_tags_box.remove(*child);
+  }
+}
+
+void BookDetails::on_book_removed(const std::string& id) {
+  if (id == m_book_id) {
+    clear();
+    m_signalBookRemoved.emit();
+  }
+}
+
+void BookDetails::on_book_list_changed() {
+  // After a full reset, check if our book still exists
+  if (m_book_id.empty()) return;
+
+  auto book_opt = m_book_list.findById(m_book_id);
+  if (!book_opt) {
+    clear();
+    m_signalBookRemoved.emit();
+  } else {
+    refresh_display();
   }
 }
 
 void BookDetails::on_open_button_clicked() {
-  if (m_book) {
-    const auto& path = m_book->filePath();
-    if (path.empty()) {
-      std::cerr << "Activated book has no file path: " << m_book->title() << "\n";
+  if (m_book_id.empty()) return;
+
+  auto book_opt = m_book_list.findById(m_book_id);
+  if (!book_opt) {
+    std::cerr << "Book no longer exists: " << m_book_id << "\n";
+    return;
+  }
+
+  const auto& path = book_opt->filePath();
+  if (path.empty()) {
+    std::cerr << "Activated book has no file path: " << book_opt->title() << "\n";
+    return;
+  }
+  try {
+    auto file = Gio::File::create_for_path(path);
+    if (!file->query_exists()) {
+      std::cerr << "Book file is missing: " << path << "\n";
       return;
     }
-    try {
-      auto file = Gio::File::create_for_path(path);
-      if (!file->query_exists()) {
-        std::cerr << "Book file is missing: " << path << "\n";
-        return;
-      }
-      // Ask the desktop to open the file with the user's default application.
-      const auto uri = Glib::filename_to_uri(path);
-      Gio::AppInfo::launch_default_for_uri(uri);
-    } catch (const Glib::Error& e) {
-      std::cerr << "Failed to open book '" << m_book->title() << "': "
-                << e.what() << "\n";
-    }
+    // Ask the desktop to open the file with the user's default application.
+    const auto uri = Glib::filename_to_uri(path);
+    Gio::AppInfo::launch_default_for_uri(uri);
+  } catch (const Glib::Error& e) {
+    std::cerr << "Failed to open book '" << book_opt->title() << "': "
+              << e.what() << "\n";
   }
 }
 

src/BookDetails.hpp

@@ -6,6 +6,7 @@
 #include <gtkmm/button.h>
 #include <gtkmm/flowbox.h>
 #include <gtkmm/entry.h>
+#include <sigc++/connection.h>
 #include <string>
 #include "Book.hpp"
 #include "DatabaseManager.hpp"
@@ -14,18 +15,26 @@
 class BookDetails : public Gtk::Box {
 public:
   BookDetails(DatabaseManager& db, BookList& book_list);
+  ~BookDetails() override;
+
   void set_book(const Book* book);
+  void clear();
+
+  // Signal emitted when the displayed book is removed from the list
+  sigc::signal<void()>& signalBookRemoved() { return m_signalBookRemoved; }
 
 private:
   void on_open_button_clicked();
   void on_add_tag_button_clicked();
   void on_remove_tag_clicked(const std::string& tag_name);
   void refresh_tags();
+  void refresh_display();
   Gtk::Widget* create_tag_chip(const std::string& tag_name);
+  void on_book_list_changed();
+  void on_book_removed(const std::string& id);
 
   DatabaseManager& m_db;
   BookList& m_book_list;
-  const Book* m_book = nullptr;
   std::string m_book_id;
 
   Gtk::Image m_cover;
@@ -35,4 +44,9 @@ private:
   Gtk::FlowBox m_tags_box;
   Gtk::Entry m_tag_entry;
   Gtk::Button m_add_tag_button;
+
+  sigc::signal<void()> m_signalBookRemoved;
+  sigc::connection m_conn_removed;
+  sigc::connection m_conn_updated;
+  sigc::connection m_conn_reset;
 };