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