From 71eeac9f1b55caa29078698d7f9cb4b6933f80f6 Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Tue, 21 Jul 2026 19:47:12 -0400 Subject: [PATCH] Fix the silent startup crash when a saved session contains navigation pins The session deserializer's _load_pins still called the LEGACY PinOverlay's clear_pins()/add_pin_button() - methods that do not exist on PinOverlayHost (window.pin_overlay since Phase 5). The AttributeError aborted the entire chat restore at launch; _handle_load_error's recovery path then crashed on the same missing method, and the app exited with no window - presenting as "sits spinning and never opens." Purely data-dependent: nothing surfaced until a session actually contained pins (created via the Pins UI earlier today), which is why every empty-session test drive passed. All five stale legacy call sites removed (deserializers._load_pins x2, deserializers._handle_load_error, ChatWindow.new_chat, and a remnant clear() in graphlink_connections.py): with the store-based Phase 5 design they are unnecessary - scene.clear() empties scene.pin_store (the overlay's reactive source of truth) and scene.add_navigation_pin() registers restored pins in it. Regression tests use a pin_overlay object with NO methods at all (a MagicMock would absorb the legacy calls and hide the bug): pins restore lands in the store without touching the overlay, and the load-error recovery path no longer raises. Verified against the real crashing session: the app now launches, restores the pinned chat, and stays up. Co-Authored-By: Claude Fable 5 --- graphlink_app/graphlink_connections.py | 8 +- .../graphlink_session/deserializers.py | 23 +++-- graphlink_app/graphlink_window.py | 4 +- .../tests/test_pins_restore_regression.py | 93 +++++++++++++++++++ 4 files changed, 115 insertions(+), 13 deletions(-) create mode 100644 graphlink_app/tests/test_pins_restore_regression.py diff --git a/graphlink_app/graphlink_connections.py b/graphlink_app/graphlink_connections.py index 62001be..1c20dfb 100644 --- a/graphlink_app/graphlink_connections.py +++ b/graphlink_app/graphlink_connections.py @@ -350,11 +350,11 @@ def remove_pin(self, pin): def clear(self): """ Clears all pins from the connection. (Note: This is a partial implementation - and seems to be a remnant, as the main scene clear handles most cleanup). + and seems to be a remnant, as the main scene clear handles most cleanup. + The legacy pin_overlay.clear_pins() call was removed with the PinOverlayHost + migration - that method no longer exists, and scene.pin_store is the + overlay's reactive source of truth.) """ - if self.window and hasattr(self.window, 'pin_overlay'): - self.window.pin_overlay.clear_pins() - self.pins.clear() self.nodes.clear() diff --git a/graphlink_app/graphlink_session/deserializers.py b/graphlink_app/graphlink_session/deserializers.py index a23690e..dc131db 100644 --- a/graphlink_app/graphlink_session/deserializers.py +++ b/graphlink_app/graphlink_session/deserializers.py @@ -540,9 +540,15 @@ def _load_notes(self, scene, notes_data): return notes_map def _load_pins(self, scene, pins_data): - if self.window and hasattr(self.window, "pin_overlay"): - self.window.pin_overlay.clear_pins() - + # No imperative pin_overlay sync here: the legacy PinOverlay's + # clear_pins()/add_pin_button() calls this method used to make do not + # exist on PinOverlayHost (Phase 5), and are unnecessary with the + # store-based design - restore_chat's scene.clear() already emptied + # scene.pin_store, add_navigation_pin() below registers each restored + # pin in it, and the overlay follows the store reactively. The stale + # calls crashed the ENTIRE chat restore (AttributeError) the first + # time a saved session actually contained pins - the app then exited + # with no window, looking like a silent startup hang. valid_records = [] for index, pin_data in enumerate(pins_data or []): try: @@ -552,7 +558,7 @@ def _load_pins(self, scene, pins_data): for record in sorted(valid_records, key=lambda item: item.sort_order): try: - pin = scene.add_navigation_pin( + scene.add_navigation_pin( QPointF(record.position[0], record.position[1]), title=record.title, note=record.note, @@ -565,8 +571,6 @@ def _load_pins(self, scene, pins_data): "warning", ) continue - if self.window and hasattr(self.window, "pin_overlay"): - self.window.pin_overlay.add_pin_button(pin) def _restore_view_state(self, chat_data): view_state = chat_data.get("view_state") @@ -602,8 +606,11 @@ def _handle_load_error(self, scene, error): self.window.message_input.setPlaceholderText("Type your message...") self.window.update_title_bar() self.window.reset_token_counter() - if hasattr(self.window, "pin_overlay") and self.window.pin_overlay: - self.window.pin_overlay.clear_pins() + # No pin_overlay.clear_pins() here: the method was the legacy + # PinOverlay's - PinOverlayHost has no such facade, so this + # RECOVERY path itself crashed with the same AttributeError it + # was recovering from. scene.clear() above already emptied + # scene.pin_store, which the overlay follows reactively. def restore_chat(self, chat, notes_data, pins_data): scene = self._scene() diff --git a/graphlink_app/graphlink_window.py b/graphlink_app/graphlink_window.py index 7d224bb..5e26409 100644 --- a/graphlink_app/graphlink_window.py +++ b/graphlink_app/graphlink_window.py @@ -1418,6 +1418,8 @@ def new_chat(self, parent_for_dialog=None): self._set_main_request_state(active=False) self._clear_loading_animation() self._clear_pending_response_preview() - if hasattr(self, 'pin_overlay') and self.pin_overlay: self.pin_overlay.clear_pins() + # No pin_overlay.clear_pins() (a legacy-PinOverlay method that does + # not exist on PinOverlayHost): scene.clear() below empties + # scene.pin_store, which the overlay follows reactively. self.session_manager.mark_context_switch(); self.session_manager.current_chat_id = None; scene.clear(); self.current_node = None; self.message_input.clear(); self.clear_attachment(); self.message_input.set_context_anchor(None); self.message_input.setPlaceholderText("Type your message..."); self.update_title_bar(); self.reset_token_counter(); return True return False diff --git a/graphlink_app/tests/test_pins_restore_regression.py b/graphlink_app/tests/test_pins_restore_regression.py new file mode 100644 index 0000000..c55fdf9 --- /dev/null +++ b/graphlink_app/tests/test_pins_restore_regression.py @@ -0,0 +1,93 @@ +"""Regression for the silent startup crash the first time a saved session +contained navigation pins. + +The session deserializer's _load_pins (and its _handle_load_error recovery +path, plus new_chat and a connections-module remnant) still called the +LEGACY PinOverlay's clear_pins()/add_pin_button() - methods that do not +exist on PinOverlayHost (Phase 5's replacement, wired as window.pin_overlay). +The AttributeError aborted the ENTIRE chat restore at launch; the recovery +handler then crashed on the same missing method, and the app exited with no +window - presenting as "the app sits spinning and never opens." It was +data-dependent: nothing surfaced until a user actually created pins and +saved, which is why empty-session test drives never caught it. + +The store-based design makes those imperative calls unnecessary: scene.clear() +empties scene.pin_store (the overlay's reactive source of truth) and +scene.add_navigation_pin() registers restored pins in it. These tests pin the +contract down with a pin_overlay object that has NO legacy methods at all - +any reintroduced legacy call fails loudly here. +""" + +import sys +import types +from pathlib import Path +from unittest.mock import MagicMock + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from PySide6.QtWidgets import QApplication + +_APP = QApplication.instance() or QApplication([]) + +from graphlink_scene import ChatScene +from graphlink_session.deserializers import SceneDeserializer + + +def _make_window_and_scene(): + window = MagicMock() + # The decisive detail: a pin_overlay with NO methods at all. The real + # PinOverlayHost has no clear_pins/add_pin_button either - a MagicMock + # would silently absorb the legacy calls and hide the regression. + window.pin_overlay = types.SimpleNamespace() + scene = ChatScene(window=window) + window.chat_view.scene.return_value = scene + return window, scene + + +_PIN_PAYLOAD = [ + { + "id": "pin-1", + "title": "First waypoint", + "note": "remember this", + "position": {"x": 120.0, "y": 340.0}, + "sort_order": 0, + }, + { + "id": "pin-2", + "title": "Second waypoint", + "note": "", + "position": {"x": -40.0, "y": 12.5}, + "sort_order": 1, + }, +] + + +class TestLoadPinsAgainstTheRealHostSurface: + def test_restoring_pins_does_not_touch_pin_overlay_and_lands_in_the_store(self): + window, scene = _make_window_and_scene() + deserializer = SceneDeserializer(window) + + deserializer._load_pins(scene, _PIN_PAYLOAD) # must not raise + + assert len(scene.pin_store.records) == 2 + titles = sorted(record.title for record in scene.pin_store.records) + assert titles == ["First waypoint", "Second waypoint"] + + def test_restoring_zero_pins_is_a_no_op(self): + window, scene = _make_window_and_scene() + deserializer = SceneDeserializer(window) + + deserializer._load_pins(scene, []) + deserializer._load_pins(scene, None) + + assert len(scene.pin_store.records) == 0 + + def test_load_error_recovery_does_not_touch_pin_overlay(self): + # The recovery path itself crashed on the same missing legacy method + # it was recovering from - the app then died with no window at all. + window, scene = _make_window_and_scene() + deserializer = SceneDeserializer(window) + + deserializer._handle_load_error(scene, RuntimeError("boom")) # must not raise + + assert window.current_node is None