diff --git a/graphlink_app/graphlink_canvas/graphlink_canvas_dialogs.py b/graphlink_app/graphlink_canvas/graphlink_canvas_dialogs.py index 46c43ee..f347671 100644 --- a/graphlink_app/graphlink_canvas/graphlink_canvas_dialogs.py +++ b/graphlink_app/graphlink_canvas/graphlink_canvas_dialogs.py @@ -11,6 +11,18 @@ class ColorPickerDialog(QDialog): """ A small, frameless pop-up dialog for selecting a color from a predefined palette. It automatically closes when the user clicks outside of it. + + Bug-scan finding: the reset/swatch buttons used to connect QPushButton.clicked + to a lambda closing over self (e.g. `lambda: self.color_selected(None, "default")`). + PySide6's GC does not reclaim a self-capturing lambda connected to a + widget-owned signal - confirmed empirically, and notably NOT limited to + custom Signals or worker threads: a stock QPushButton.clicked calling a + plain method (not even a Signal.emit) forms the identical GC-invisible + cycle - so this dialog leaked forever every time it was opened. Fixed by + connecting the default button to a small dedicated bound method, and + storing each swatch's color data via QWidget.setProperty() so every swatch + button can share one bound-method dispatcher that reads + self.sender().property(...). """ def __init__(self, parent=None): super().__init__(parent) @@ -57,7 +69,7 @@ def __init__(self, parent=None): default_btn = QPushButton("Reset to Default") default_btn.setIcon(qta.icon('fa5s.undo', color='white')) - default_btn.clicked.connect(lambda: self.color_selected(None, "default")) + default_btn.clicked.connect(self._on_default_clicked) main_layout.addWidget(default_btn) def create_section(title, color_type, names_list): @@ -93,8 +105,9 @@ def create_section(title, color_type, names_list): QPushButton:hover {{ border: 2px solid #FFFFFF; }} """ btn.setStyleSheet(style) - btn.clicked.connect(lambda checked, c=color_data: self.color_selected(c["color"], c["type"])) - + btn.setProperty("frame_color_data", color_data) + btn.clicked.connect(self._on_swatch_clicked) + grid_layout.addWidget(btn, row, col) col = (col + 1) % 5 if col == 0: row += 1 @@ -150,6 +163,13 @@ def changeEvent(self, event): self.close() super().changeEvent(event) + def _on_default_clicked(self): + self.color_selected(None, "default") + + def _on_swatch_clicked(self): + color_data = self.sender().property("frame_color_data") + self.color_selected(color_data["color"], color_data["type"]) + def color_selected(self, color, color_type): self.selected_color = color self.selected_type = color_type diff --git a/graphlink_app/graphlink_nodes/graphlink_node_chat_menu.py b/graphlink_app/graphlink_nodes/graphlink_node_chat_menu.py index e55730e..9d05b0f 100644 --- a/graphlink_app/graphlink_nodes/graphlink_node_chat_menu.py +++ b/graphlink_app/graphlink_nodes/graphlink_node_chat_menu.py @@ -11,6 +11,16 @@ class ChatNodeContextMenu(QMenu): """ A comprehensive context menu for ChatNode, providing access to text manipulation, AI actions (summaries, explainers, charts), and organizational tools. + + Bug-scan finding: the export/reveal/chart-type actions used to connect + QAction.triggered to a lambda closing over self (e.g. + `lambda: self._handle_export('txt')`). PySide6's GC does not reclaim a + self-capturing lambda connected to a widget-owned signal (confirmed + empirically - a bound-method connection is reclaimed fine), so this menu + - and the ChatNode it stores via self.node - leaked forever on EVERY + right-click. Fixed by storing the per-action variant via + QAction.setData() and connecting every action to one shared bound-method + dispatcher that reads self.sender().data(). """ def __init__(self, node, parent=None): @@ -54,7 +64,8 @@ def __init__(self, node, parent=None): node_label = docked_node.docked_label() if hasattr(docked_node, "docked_label") else docked_node.__class__.__name__ reveal_action = QAction(node_label, undock_menu) reveal_action.setIcon(qta.icon('fa5s.expand-arrows-alt', color='white')) - reveal_action.triggered.connect(lambda checked=False, target=docked_node: self.undock_node(target)) + reveal_action.setData(docked_node) + reveal_action.triggered.connect(self._on_reveal_action_triggered) undock_menu.addAction(reveal_action) self.addMenu(undock_menu) @@ -98,7 +109,8 @@ def __init__(self, node, parent=None): for title, chart_type, icon in chart_types: action = QAction(title, chart_menu) action.setIcon(qta.icon(icon, color='white')) - action.triggered.connect(lambda checked, t=chart_type: self.generate_chart(t)) + action.setData(chart_type) + action.triggered.connect(self._on_chart_action_triggered) chart_menu.addAction(action) self.addMenu(chart_menu) @@ -133,27 +145,41 @@ def create_export_menu(self): export_menu.setIcon(qta.icon('fa5s.file-export', color='white')) txt_action = QAction("Text File (.txt)", self) - txt_action.triggered.connect(lambda: self._handle_export('txt')) + txt_action.setData('txt') + txt_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(txt_action) md_action = QAction("Markdown File (.md)", self) - md_action.triggered.connect(lambda: self._handle_export('md')) + md_action.setData('md') + md_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(md_action) html_action = QAction("HTML Document (.html)", self) - html_action.triggered.connect(lambda: self._handle_export('html')) + html_action.setData('html') + html_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(html_action) docx_action = QAction("Word Document (.docx)", self) - docx_action.triggered.connect(lambda: self._handle_export('docx')) + docx_action.setData('docx') + docx_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(docx_action) pdf_action = QAction("PDF Document (.pdf)", self) - pdf_action.triggered.connect(lambda: self._handle_export('pdf')) + pdf_action.setData('pdf') + pdf_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(pdf_action) return export_menu + def _on_reveal_action_triggered(self): + self.undock_node(self.sender().data()) + + def _on_chart_action_triggered(self): + self.generate_chart(self.sender().data()) + + def _on_export_action_triggered(self): + self._handle_export(self.sender().data()) + def _handle_export(self, file_format): exporter = Exporter() content = self.node.text diff --git a/graphlink_app/graphlink_nodes/graphlink_node_code_menu.py b/graphlink_app/graphlink_nodes/graphlink_node_code_menu.py index a9b2966..0c3f18e 100644 --- a/graphlink_app/graphlink_nodes/graphlink_node_code_menu.py +++ b/graphlink_app/graphlink_nodes/graphlink_node_code_menu.py @@ -7,7 +7,17 @@ class CodeNodeContextMenu(QMenu): - """Context menu for CodeNode, providing copy, export, and regenerate actions.""" + """Context menu for CodeNode, providing copy, export, and regenerate actions. + + Bug-scan finding: the export actions used to connect QAction.triggered to + a lambda closing over self (e.g. `lambda: self._handle_export('txt')`). + PySide6's GC does not reclaim a self-capturing lambda connected to a + widget-owned signal (confirmed empirically - a bound-method connection is + reclaimed fine), so this menu - and the CodeNode it stores via self.node - + leaked forever on EVERY right-click. Fixed by storing the format via + QAction.setData() and connecting every export action to one shared + bound-method dispatcher that reads self.sender().data(). + """ def __init__(self, node, parent=None): super().__init__(parent) @@ -49,27 +59,35 @@ def create_export_menu(self): export_menu.setIcon(qta.icon('fa5s.file-export', color='white')) py_action = QAction("Python Script (.py)", self) - py_action.triggered.connect(lambda: self._handle_export('py')) + py_action.setData('py') + py_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(py_action) txt_action = QAction("Text File (.txt)", self) - txt_action.triggered.connect(lambda: self._handle_export('txt')) + txt_action.setData('txt') + txt_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(txt_action) md_action = QAction("Markdown File (.md)", self) - md_action.triggered.connect(lambda: self._handle_export('md')) + md_action.setData('md') + md_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(md_action) html_action = QAction("HTML Document (.html)", self) - html_action.triggered.connect(lambda: self._handle_export('html')) + html_action.setData('html') + html_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(html_action) pdf_action = QAction("PDF Document (.pdf)", self) - pdf_action.triggered.connect(lambda: self._handle_export('pdf')) + pdf_action.setData('pdf') + pdf_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(pdf_action) return export_menu + def _on_export_action_triggered(self): + self._handle_export(self.sender().data()) + def _handle_export(self, file_format): exporter = Exporter() content = self.node.code diff --git a/graphlink_app/graphlink_nodes/graphlink_node_document_menu.py b/graphlink_app/graphlink_nodes/graphlink_node_document_menu.py index 067153a..cb15e94 100644 --- a/graphlink_app/graphlink_nodes/graphlink_node_document_menu.py +++ b/graphlink_app/graphlink_nodes/graphlink_node_document_menu.py @@ -10,7 +10,17 @@ class DocumentNodeContextMenu(QMenu): - """Context menu for uploaded file attachments.""" + """Context menu for uploaded file attachments. + + Bug-scan finding: the export actions used to connect QAction.triggered to + a lambda closing over self (e.g. `lambda: self._handle_export('txt')`). + PySide6's GC does not reclaim a self-capturing lambda connected to a + widget-owned signal (confirmed empirically - a bound-method connection is + reclaimed fine), so this menu - and the node it stores via self.node - + leaked forever on EVERY right-click. Fixed by storing the format via + QAction.setData() and connecting every export action to one shared + bound-method dispatcher that reads self.sender().data(). + """ def __init__(self, node, parent=None): super().__init__(parent) @@ -66,27 +76,35 @@ def create_export_menu(self): export_menu.setIcon(qta.icon('fa5s.file-export', color='white')) txt_action = QAction("Text File (.txt)", self) - txt_action.triggered.connect(lambda: self._handle_export('txt')) + txt_action.setData('txt') + txt_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(txt_action) md_action = QAction("Markdown File (.md)", self) - md_action.triggered.connect(lambda: self._handle_export('md')) + md_action.setData('md') + md_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(md_action) html_action = QAction("HTML Document (.html)", self) - html_action.triggered.connect(lambda: self._handle_export('html')) + html_action.setData('html') + html_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(html_action) docx_action = QAction("Word Document (.docx)", self) - docx_action.triggered.connect(lambda: self._handle_export('docx')) + docx_action.setData('docx') + docx_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(docx_action) pdf_action = QAction("PDF Document (.pdf)", self) - pdf_action.triggered.connect(lambda: self._handle_export('pdf')) + pdf_action.setData('pdf') + pdf_action.triggered.connect(self._on_export_action_triggered) export_menu.addAction(pdf_action) return export_menu + def _on_export_action_triggered(self): + self._handle_export(self.sender().data()) + def _handle_export(self, file_format): exporter = Exporter() content = self.node.content diff --git a/graphlink_app/tests/test_context_menu_and_dialog_lambda_leaks.py b/graphlink_app/tests/test_context_menu_and_dialog_lambda_leaks.py new file mode 100644 index 0000000..756c943 --- /dev/null +++ b/graphlink_app/tests/test_context_menu_and_dialog_lambda_leaks.py @@ -0,0 +1,232 @@ +"""Bug-scan finding: ChatNodeContextMenu, CodeNodeContextMenu, +DocumentNodeContextMenu, and ColorPickerDialog connected several +QAction.triggered/QPushButton.clicked signals to a lambda closing over +self (e.g. `lambda: self._handle_export('txt')`, or - inside a loop - +`lambda checked, t=chart_type: self.generate_chart(t)`). + +PySide6's garbage collector does not reclaim a self-capturing lambda +connected to a widget-owned signal - confirmed empirically (see the +worker-owning-node fix in PR #93): a bound-method connection to the same +signal IS reclaimed fine, a lambda is not. And it isn't limited to custom +Qt Signals or worker threads either - ColorPickerDialog's leak comes from +a stock QPushButton.clicked calling a plain method, no Signal.emit +involved at all. + +So every one of these menus/dialogs - and, for the three context menus, +the canvas node each one stores via self.node - leaked forever, for the +rest of the process, on every single right-click or "change color" popup. + +Fixed by moving the per-instance variant data onto the widget itself +(QAction.setData() / QWidget.setProperty()) and connecting every +action/button to ONE shared bound-method dispatcher that reads +self.sender().data()/.property(...). A bound-method connection doesn't +trip the same GC-invisible cycle. + +These tests use weakref + gc.collect() against REAL menu/dialog + REAL +node instances (never a MagicMock in place of the object under GC test - +a MagicMock's own internal bookkeeping can itself look like a leak or +mask one, so it would be the wrong tool here) to prove collectibility +directly, matching the convention established in +test_plugin_node_dispose_lifecycle.py's TestArtifactNodeDelBackstop / +TestDisposeBreaksTheWindowActionsSignalCycle classes. +""" + +import gc +import sys +import weakref +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_canvas.graphlink_canvas_dialogs import ColorPickerDialog +from graphlink_nodes.graphlink_node_chat_menu import ChatNodeContextMenu +from graphlink_nodes.graphlink_node_code_menu import CodeNodeContextMenu +from graphlink_nodes.graphlink_node_document_menu import DocumentNodeContextMenu +from graphlink_scene import ChatScene + + +def _make_scene(): + window = MagicMock() + return ChatScene(window=window) + + +class TestChatNodeContextMenuDoesNotLeak: + def _build(self): + scene = _make_scene() + node = scene.add_chat_node("hello world", is_user=True) + menu = ChatNodeContextMenu(node) + return scene, weakref.ref(menu), weakref.ref(node) + + def test_menu_and_node_are_collectible_after_construction(self): + scene, menu_ref, node_ref = self._build() + del scene + gc.collect() + + assert menu_ref() is None, "ChatNodeContextMenu leaked (self-capturing lambda cycle)" + assert node_ref() is None, "ChatNode leaked via the menu's self.node reference" + + def test_export_actions_still_dispatch_to_the_right_format(self): + scene = _make_scene() + node = scene.add_chat_node("hello world", is_user=True) + menu = ChatNodeContextMenu(node) + menu._handle_export = MagicMock() + + export_menu = menu.create_export_menu() + formats_seen = [] + for action in export_menu.actions(): + formats_seen.append(action.data()) + action.trigger() + + assert formats_seen == ["txt", "md", "html", "docx", "pdf"] + assert menu._handle_export.call_args_list == [ + ((fmt,),) for fmt in ("txt", "md", "html", "docx", "pdf") + ] + + def test_reveal_action_carries_the_docked_node_as_its_data(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = scene.add_chat_node("child", is_user=False, parent_node=parent) + docked = scene.add_chat_node("docked", is_user=False, parent_node=parent) + node.get_docked_child_nodes = lambda: [docked] + + menu = ChatNodeContextMenu(node) + menu.undock_node = MagicMock() + + undock_menu = None + for action in menu.actions(): + sub = action.menu() + if sub is not None and sub.title() == "Reveal Docked Items": + undock_menu = sub + break + assert undock_menu is not None, "Reveal Docked Items submenu not found" + + reveal_actions = undock_menu.actions() + assert len(reveal_actions) == 1 + assert reveal_actions[0].data() is docked + + reveal_actions[0].trigger() + menu.undock_node.assert_called_once_with(docked) + + def test_chart_type_actions_dispatch_the_right_type(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = scene.add_chat_node("child", is_user=False, parent_node=parent) + menu = ChatNodeContextMenu(node) + menu.generate_chart = MagicMock() + + chart_menu = None + for action in menu.actions(): + sub = action.menu() + if sub is not None and sub.title() == "Generate Chart": + chart_menu = sub + break + assert chart_menu is not None, "Generate Chart submenu not found" + + types_seen = [a.data() for a in chart_menu.actions()] + assert types_seen == ["bar", "line", "histogram", "pie", "sankey"] + + chart_menu.actions()[0].trigger() + menu.generate_chart.assert_called_once_with("bar") + + +class TestCodeNodeContextMenuDoesNotLeak: + def _build(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=False) + node = scene.add_code_node("print(1)", "python", parent) + menu = CodeNodeContextMenu(node) + return scene, weakref.ref(menu), weakref.ref(node) + + def test_menu_and_node_are_collectible_after_construction(self): + scene, menu_ref, node_ref = self._build() + del scene + gc.collect() + + assert menu_ref() is None, "CodeNodeContextMenu leaked (self-capturing lambda cycle)" + assert node_ref() is None, "CodeNode leaked via the menu's self.node reference" + + def test_export_actions_still_dispatch_to_the_right_format(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=False) + node = scene.add_code_node("print(1)", "python", parent) + menu = CodeNodeContextMenu(node) + menu._handle_export = MagicMock() + + export_menu = menu.create_export_menu() + formats_seen = [a.data() for a in export_menu.actions()] + export_menu.actions()[0].trigger() + + assert formats_seen == ["py", "txt", "md", "html", "pdf"] + menu._handle_export.assert_called_once_with("py") + + +class TestDocumentNodeContextMenuDoesNotLeak: + def _build(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = scene.add_document_node("file.txt", "content", parent, attachment_kind="document") + menu = DocumentNodeContextMenu(node) + return scene, weakref.ref(menu), weakref.ref(node) + + def test_menu_and_node_are_collectible_after_construction(self): + scene, menu_ref, node_ref = self._build() + del scene + gc.collect() + + assert menu_ref() is None, "DocumentNodeContextMenu leaked (self-capturing lambda cycle)" + assert node_ref() is None, "DocumentNode leaked via the menu's self.node reference" + + def test_export_actions_still_dispatch_to_the_right_format(self): + scene = _make_scene() + parent = scene.add_chat_node("parent", is_user=True) + node = scene.add_document_node("file.txt", "content", parent, attachment_kind="document") + menu = DocumentNodeContextMenu(node) + menu._handle_export = MagicMock() + + export_menu = menu.create_export_menu() + formats_seen = [a.data() for a in export_menu.actions()] + export_menu.actions()[3].trigger() + + assert formats_seen == ["txt", "md", "html", "docx", "pdf"] + menu._handle_export.assert_called_once_with("docx") + + +class TestColorPickerDialogDoesNotLeak: + def test_dialog_is_collectible_after_construction(self): + def build(): + dialog = ColorPickerDialog(None) + return weakref.ref(dialog) + + ref = build() + gc.collect() + + assert ref() is None, "ColorPickerDialog leaked (self-capturing lambda cycle)" + + def test_default_button_selects_default(self): + dialog = ColorPickerDialog(None) + dialog._on_default_clicked() + + assert dialog.get_selected_color() == (None, "default") + + def test_a_swatch_button_selects_its_own_color_data(self): + from PySide6.QtWidgets import QPushButton + + dialog = ColorPickerDialog(None) + # Find a real swatch button by its stored property (the reset button + # has no such property, so this can't accidentally match it). + swatch_buttons = [ + btn for btn in dialog.findChildren(QPushButton) + if btn.property("frame_color_data") is not None + ] + assert swatch_buttons, "no swatch buttons found" + + target = swatch_buttons[0] + expected = target.property("frame_color_data") + target.click() + + assert dialog.get_selected_color() == (expected["color"], expected["type"])