Fix crash dismissing a covered modal, and stop catalog error floods

CHANGELOG.md

@@ -9,6 +9,10 @@ All notable changes to this project will be documented in this file.
 - DuckDB geometry columns now show as text, like `POINT (-117.93367 34.34613)`, instead of as raw bytes, in the Results Viewer and everywhere else; the spatial extension's types also get a `geo` label in the Data Catalog ([#302](https://github.com/tconbeer/harlequin/issues/302)).
 - Press `F7` (bindable as `toggle_keys_panel`) to show or hide the Keys panel, which is restyled to match Harlequin's other panels and can now be focused and scrolled ([#753](https://github.com/tconbeer/harlequin/issues/753)).
 
+### Bug Fixes
+
+- Fixes a crash when closing an error modal. A lost connection (e.g., a closed SSH tunnel) now shows one Data Catalog error instead of one for every node Harlequin tried to load ([#1171](https://github.com/tconbeer/harlequin/issues/1171)).
+
 ### Refactoring
 
 - Harlequin and `hsql` now declare their own CLI options in one place, where each option also says which command takes it and how it may be used ([#1145](https://github.com/tconbeer/harlequin/issues/1145)).

src/harlequin/app.py

@@ -454,7 +454,6 @@ def push_screen(  # type: ignore[override]
         if self.editor is not None and self.editor._has_focus_within:
             self.editor.pause_blink(visible=True)
 
-        ## TODO: PREVENT DUPLICATE SCREENS HERE.
         return super().push_screen(  # type: ignore[no-any-return,call-overload]
             screen,
             callback=callback,
@@ -1625,6 +1624,19 @@ def _get_selected_queries(self) -> list[str]:
         return self.editor.selected_queries()
 
     def _push_error_modal(self, title: str, header: str, error: BaseException) -> None:
+        error_text = str(error)
+        for screen in self.screen_stack:
+            if (
+                isinstance(screen, ErrorModal)
+                and screen.title == title
+                and screen.header == header
+                and screen.text == error_text
+            ):
+                # one failure reported many times is one modal to dismiss
+                self.log.warning(
+                    "Repeated error modal suppressed", title=title, error=error
+                )
+                return
         self.push_screen(
             ErrorModal(
                 title=title,

src/harlequin/components/confirm_modal.py

@@ -3,11 +3,12 @@
 from textual import on
 from textual.app import ComposeResult
 from textual.containers import Horizontal, Vertical
-from textual.screen import ModalScreen
 from textual.widgets import Button, Label
 
+from harlequin.components.modal import HarlequinModal
 
-class ConfirmModal(ModalScreen[bool]):
+
+class ConfirmModal(HarlequinModal[bool]):
     def __init__(self, prompt: str) -> None:
         super().__init__()
         self.prompt = prompt

src/harlequin/components/data_catalog/database_tree.py

@@ -101,6 +101,8 @@ def __init__(
         self._node_ids: dict[int, NodeID] = {}
         """The id of the node built for each item, keyed by id(item)."""
         self._prefetch_timer: Timer | None = None
+        self._prefetch_paused = False
+        """Prevents the loader from prefetching new nodes."""
         super().__init__(
             label="Root",
             data=CatalogItem(
@@ -216,6 +218,8 @@ def _queue_nodes_in_view(self) -> None:
         gets.
         """
         self._prefetch_timer = None
+        if self._prefetch_paused:
+            return
         first, last = self._prefetch_window()
         lines = self._tree_lines
         for line_no in range(first, min(last + 1, len(lines))):
@@ -241,7 +245,7 @@ def _queue_named_items(self) -> None:
         columns of the relation the buffer also names -- so this runs again
         whenever the loader delivers more of the catalog.
         """
-        if not self._symbol_names or self.root.data is None:
+        if not self._symbol_names or self.root.data is None or self._prefetch_paused:
             return
         queued = 0
         frontier: deque[CatalogItem] = deque(self.root.data.children)
@@ -274,6 +278,7 @@ def reload(self) -> AwaitComplete:
         """
         # Orphan the old queue...
         self._load_queue = PriorityQueue()
+        self._prefetch_paused = False
         self._queued_priority.clear()
         self._loading.clear()
         self._node_ids.clear()
@@ -488,9 +493,11 @@ def _load_children(self, item: CatalogItem) -> list[CatalogItem]:
             try:
                 children = list(item.fetch_children())
             except BaseException as e:
+                self._prefetch_paused = True
                 self.post_message(self.CatalogError(catalog_type="database", error=e))
                 return []
             else:
+                self._prefetch_paused = False
                 item.children = children
                 self.post_message(NewCatalogItems(parent=item, items=children))
             finally:
@@ -526,11 +533,16 @@ async def _loader(self) -> None:
                     # (or has since been loaded), so it is handled elsewhere.
                     continue
                 del self._queued_priority[key]
+                if priority != DEMAND_PRIORITY and self._prefetch_paused:
+                    # queued before a fetch failed; the next scan re-queues it
+                    # once a fetch succeeds
+                    continue
                 if priority == PREFETCH_PRIORITY and not self._is_in_view(item):
                     # scrolled or collapsed out of view before we got to it, so
                     # the speculation no longer pays for itself.
                     continue
                 self._loading.add(key)
+                was_paused = self._prefetch_paused
                 try:
                     # Spin up a short-lived thread that will load the item's
                     # children. The tree lock is deliberately not held here: an
@@ -548,6 +560,9 @@ async def _loader(self) -> None:
                     continue
                 finally:
                     self._loading.discard(key)
+                if was_paused and not self._prefetch_paused:
+                    # a fetch succeeded, so re-queue what the pause skipped
+                    self._schedule_prefetch_scan()
                 # the children we just loaded may include more of what the
                 # buffer names, a level deeper
                 self._queue_named_items()

src/harlequin/components/debug_info.py

@@ -8,9 +8,9 @@
 from textual import events
 from textual.app import ComposeResult
 from textual.containers import VerticalScroll
-from textual.screen import ModalScreen
 from textual.widgets import Collapsible, Markdown, Static
 
+from harlequin.components.modal import HarlequinModal
 from harlequin.components.text_modal import VerticalSuppressClicks
 from harlequin.config import Config, Profile
 from harlequin.options import AbstractOption
@@ -242,7 +242,7 @@ def parse_info(self) -> List[DebugWidget]:
         ]
 
 
-class DebugInfoScreen(ModalScreen):
+class DebugInfoScreen(HarlequinModal[None]):
     def __init__(
         self,
         harlequin_details: List[DebugWidget],
@@ -327,7 +327,7 @@ def move_focus(self, direction: int) -> None:
 
     def on_key(self, event: events.Key) -> None:
         if event.key == "escape":
-            self.app.pop_screen()
+            self.dismiss()
             event.stop()
         elif event.key == "pageup":
             container = self.query_one("#modal_outer")
@@ -339,4 +339,4 @@ def on_key(self, event: events.Key) -> None:
             event.stop()
 
     def on_click(self) -> None:
-        self.app.pop_screen()
+        self.dismiss()

src/harlequin/components/export_screen.py

@@ -8,12 +8,12 @@
 from textual.app import ComposeResult
 from textual.containers import Horizontal, Vertical, VerticalScroll
 from textual.css.query import QueryError
-from textual.screen import ModalScreen
 from textual.widget import Widget
 from textual.widgets import Button, Input, Label, Select, Static
 from textual_fastdatatable.backend import ArrowBackend
 from textual_textarea import PathInput
 
+from harlequin.components.modal import HarlequinModal
 from harlequin.components.results_viewer import ResultsTable
 from harlequin.components.text_modal import ErrorModal
 from harlequin.exception import HarlequinCopyError
@@ -24,7 +24,7 @@
 
 
 def export_callback(
-    screen_data: Tuple[Path, str, ExportOptions],
+    screen_data: Tuple[Path, str, ExportOptions] | None,
     table: ResultsTable,
     success_callback: Callable[[], None],
     error_callback: Callable[[Exception], None],
@@ -44,7 +44,11 @@ def export_callback(
     nothing else, or an empty array. That is a true account of what the query
     returned, and it is what tells a reader "nothing matched" apart from
     "the query failed".
+
+    None is a dialog the user cancelled.
     """
+    if screen_data is None:
+        return
     path, format_name, options = screen_data
     try:
         assert isinstance(table.backend, ArrowBackend)
@@ -128,7 +132,7 @@ def _get_option_value_by_name(self, name: str) -> Any | None:
             return None
 
 
-class ExportScreen(ModalScreen[Tuple[Path, str, ExportOptions]]):
+class ExportScreen(HarlequinModal[Tuple[Path, str, ExportOptions]]):
     def __init__(
         self,
         formats: list[HarlequinCopyFormat],
@@ -189,14 +193,14 @@ def on_mount(self) -> None:
 
     def on_key(self, event: events.Key) -> None:
         if event.key == "escape":
-            self.app.pop_screen()
+            self.dismiss()
 
     def on_button_pressed(self, event: Button.Pressed) -> None:
         button = event.button
         if button.id == "export":
             self._export()
         else:
-            self.app.pop_screen()
+            self.dismiss()
 
     def on_input_changed(self, event: Input.Changed) -> None:
         event.stop()

src/harlequin/components/help_screen.py

@@ -5,13 +5,13 @@
 from textual import events
 from textual.app import ComposeResult
 from textual.containers import VerticalScroll
-from textual.screen import ModalScreen
 from textual.widgets import Markdown, Static
 
+from harlequin.components.modal import HarlequinModal
 from harlequin.components.text_modal import VerticalSuppressClicks
 
 
-class HelpScreen(ModalScreen):
+class HelpScreen(HarlequinModal[None]):
     header_text = """
         Welcome to Harlequin! This screen contains a small subset of the online
         docs, available at https://harlequin.sh/docs/getting-started
@@ -73,7 +73,7 @@ def on_key(self, event: events.Key) -> None:
         elif event.key == "pagedown":
             self.body.scroll_page_down()
         else:
-            self.app.pop_screen()
+            self.dismiss()
 
     def on_click(self) -> None:
-        self.app.pop_screen()
+        self.dismiss()

src/harlequin/components/history_screen.py

@@ -10,12 +10,12 @@
 from textual.app import ComposeResult
 from textual.containers import Horizontal
 from textual.message import Message
-from textual.screen import ModalScreen
 from textual.timer import Timer
 from textual.widgets import Footer, Input, OptionList, Select
 from textual.widgets.option_list import Option
 from textual_textarea import TextEditor
 
+from harlequin.components.modal import HarlequinModal
 from harlequin.history import History, QueryExecution
 from harlequin.messages import WidgetMounted
 from harlequin.query_log import PROGRAMS, STATUSES
@@ -74,7 +74,7 @@ class QueryPreview(TextEditor, inherit_bindings=False):
     """
 
 
-class HistoryScreen(ModalScreen[str]):
+class HistoryScreen(HarlequinModal[str]):
     COMPONENT_CLASSES: ClassVar[set[str]] = {
         "history-screen--error-label",
     }
@@ -193,7 +193,7 @@ def action_toggle_filters(self) -> None:
     def action_cancel(self) -> None:
         """Empty the filter, put the controls away, or leave the screen."""
         if not self._filters_have_focus:
-            self.app.pop_screen()
+            self.dismiss()
         elif self.filter_input.value:
             self.filter_input.value = ""
         else:

src/harlequin/components/modal.py

@@ -0,0 +1,19 @@
+from __future__ import annotations
+
+from textual.await_complete import AwaitComplete
+from textual.screen import ModalScreen, ScreenResultType
+
+
+class HarlequinModal(ModalScreen[ScreenResultType]):
+    """Base for Harlequin's modals, which only dismiss themselves from the top.
+
+    Textual's `dismiss()` spends this screen's result and then pops whatever
+    screen is on top, so an event this modal received before another covered it
+    (an error modal pushed by a worker, say) would pop that one instead, and the
+    next dismiss would raise `InvalidStateError`.
+    """
+
+    def dismiss(self, result: ScreenResultType | None = None) -> AwaitComplete:
+        if not self.is_active:
+            return AwaitComplete()
+        return super().dismiss(result)

src/harlequin/components/text_modal.py

@@ -7,9 +7,10 @@
 from textual import events
 from textual.app import ComposeResult
 from textual.containers import Vertical, VerticalScroll
-from textual.screen import ModalScreen
 from textual.widgets import Static
 
+from harlequin.components.modal import HarlequinModal
+
 
 class VerticalSuppressClicks(Vertical):
     def on_click(self, message: events.Click) -> None:
@@ -22,7 +23,7 @@ def on_click(self, message: events.Click) -> None:
         cast("TextModal", self.screen).copy()
 
 
-class TextModal(ModalScreen[None]):
+class TextModal(HarlequinModal[None]):
     """Base for the modals that show a block of text -- a result cell's whole
     value, an error message -- in a scrollable pane. The arrow and page keys
     scroll it, clicking the text or pressing c copies it, and any other key