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

[tconbeer]

Closes #1171

What are the key elements of this solution?

• HarlequinModal, a shared base whose dismiss() only acts when the modal is on top (components/modal.py).
  • Textual's Screen.dismiss() spends this screen's result callback but pops whatever is on top. An event a modal received before another covered it therefore popped the wrong modal and left this one up with its result spent. The next dismiss raised asyncio.exceptions.InvalidStateError, which is the traceback in the report.
  • Every Harlequin modal now derives from this base: TextModal/ErrorModal, ConfirmModal, HistoryScreen, ExportScreen, HelpScreen and DebugInfoScreen.
  • The modal-local app.pop_screen() calls now go through self.dismiss(). Before, they could drop an error or confirm modal unseen.
  • export_callback returns on None, because dismiss() runs the callback where pop_screen() didn't.
• A failed catalog fetch pauses speculative loading (database_tree.py).
  • After one fetch_children() fails, the viewport prefetch and the buffer-symbol loads stop queuing, and already-queued speculative items drain without a fetch.
  • Nodes the user expands are still fetched. A successful fetch resumes prefetch, and a catalog refresh resets the pause.
  • Before this, a dead connection failed once for every unloaded node within about 20 lines of the viewport. When failures were slow (connection timeouts), an identical modal reappeared after every dismissal. That was the "froze and wouldn't let me exit" in the report.
• _push_error_modal doesn't push a duplicate of an ErrorModal still on the stack (same title, header and message), and logs the one it skipped. This is a backstop for any other source of repeated errors.

Why did you design your solution this way? Did you assess any alternatives? Are there tradeoffs?

• The guard lives on the modal because only the modal knows which screen it meant to dismiss. Overriding Harlequin.pop_screen can't tell.
• The flood is stopped in the loader, where it starts. The modal dedup alone only merged errors while the first modal was still on screen.
• Tradeoff: a stale action on a covered modal is dropped rather than replayed. For example, a Yes that landed as an error modal covered the prompt: the user answers again once the error is dismissed.
• Out of scope: a tunnel reused with --ssh-allow-reuse is never watched, and catalog fetches don't go through _connection_for_worker. So after a drop, expanding a node still fails; a query or a catalog refresh recovers the tunnel and rebuilds the tree.
• Upstream: Textual's dismiss() popping a screen other than self looks worth an issue against textualize/textual (8.2.8 still does it).

Does this PR require a change to Harlequin's docs?

• No.
• Yes, and I have opened a PR at tconbeer/harlequin-web.
• Yes; I haven't opened a PR, but the gist of the change is: ...

Did you add or update tests for this change?

• Yes.
• No, I believe tests aren't necessary.
• No, I need help with testing this change.

I checked that each new test fails with its fix reverted:

• test_key_queued_for_a_covered_error_modal_does_not_crash: without the guard, it fails at its first assertion because the wrong modal was popped. Its later steps are what raise InvalidStateError.
• test_click_queued_for_a_covered_error_modal_does_not_dismiss_either and test_confirm_pressed_under_an_error_modal_waits_for_the_user: both fail without the guard.
• test_every_modal_dismisses_only_from_the_top: fails if any ModalScreen in harlequin.components doesn't derive from HarlequinModal.
• test_failed_fetch_pauses_prefetch_until_one_succeeds: goes through the real loader, and fails without the pause or without the resume scan.
• test_identical_errors_raise_one_modal: covers the modal dedup.

make check is green: 2131 passed, plus the py3.12 tests, mypy and import-linter.

Please complete the following checklist:

• I have added an entry to CHANGELOG.md, under the [Unreleased] section heading. That entry references the issue closed by this PR.
• I acknowledge Harlequin's MIT license. I do not own my contribution.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WFBeRM1DiYfXDTAtBawAPZ

[tconbeer]

Scope

Lenses: correctness, and whether other scenarios can cause the same crash. There were three narrow passes: the TextModal guard and its tests, a sweep of every other screen that dismisses, and the error-modal dedup and the catalog path behind it. I also did a short comments/conventions read.

Verdict

The diagnosis is right, and so is the TextModal fix. Textual's Screen.dismiss() spends this screen's result callback and then pops whatever is on top. The key test fails on main and passes here, and both new tests passed 10 out of 10 runs.

The fix isn't complete for the crash class, though. ConfirmModal, HistoryScreen and ExportScreen crash the same way, with InvalidStateError at textual/screen.py:141 and the app exiting, when an error modal is pushed over them while a button press or selection is still in flight. I reproduced all three on this head. A dropped tunnel, the #1171 scenario, is exactly when error modals arrive at unpredictable moments. I'd move the guard into a shared modal base (details inline).

Findings, most severe first

1. Should fix: the same crash in confirm_modal.py:31,34, history_screen.py:295 and export_screen.py:269.
   • Four modal handlers (help, debug info, export and history screens) call app.pop_screen() and can drop an error or confirm modal unseen.
   • The fix: a shared dismiss() guard. Details are on text_modal.py:78.
2. Should fix: the on_click guard has no test.
3. Follow-up: the dedup only merges errors while the first modal is on screen. The catalog loader keeps fetching nearby nodes on a dead connection, so slow failures (30 s timeouts) bring the identical modal back after every dismissal. The source-level fix belongs in DatabaseTree._loader. The dedup key also silently depends on adapters keeping the node name out of str(error).
4. Nit: the key test fails with an assertion rather than InvalidStateError; the PR body should say so. The dedup test bypasses the real loader.
5. Nit: the pre-existing ## TODO: PREVENT DUPLICATE SCREENS HERE. in Harlequin.push_screen is now half-addressed one layer down; either resolve it or reword it. The changelog entry slightly overclaims ("shows a repeated error once"; see 3).

Comments otherwise follow AGENTS.md: they explain what and why, with no history.

The root cause of #1171 is out of scope, as the PR says: a reused tunnel is never watched, and catalog fetches bypass _connection_for_worker. It's worth its own issue.

Not checked

• I didn't run anything against a real Redshift or SSH tunnel. The redshift_connector error text and whether a half-open socket hangs forever come from reading harlequin-redshift 0.1.0, not from running it.
• I didn't examine harlequin-mysql, or the S3 tree's CatalogError path.
• I didn't repro the double-key case in harlequin --keys (keys_app.py:252 InputModal). By reading, it's low risk, since that app pushes no async modals.
• I didn't examine Textual's command palette or other system screens stacked over a TextModal.
• I didn't run make check. I ran the new tests (10 times each), test_app.py, test_results_viewer.py, test_data_catalog.py and test_ssh_recovery.py under xdist, on Python 3.10 on Linux only.
• I didn't check Windows or macOS event timing.
• Whether Textual's closed issue #4884 ("InvalidStateError when dismissing Screen") settled this upstream is unverified.

The merge call is yours.

---

Generated by Claude Code