From 696b6a03d57d50d4707c7a8d6b579f70d609e7f7 Mon Sep 17 00:00:00 2001 From: hellices Date: Fri, 7 Aug 2026 10:07:49 +0900 Subject: [PATCH 1/2] fix(ui): stop the pick modal dying when it mounts before its options `PickScreen.on_mount` called `query_one(OptionList)` unconditionally, but Textual does not guarantee that a screen's composed children are queryable by the time `on_mount` runs. When they are not, the query raises `NoMatches` and takes the app down mid-open. This has been latent since #6 and only shows up under load, which is why it surfaced on CI rather than locally: `test (3.12)` on main failed with `No nodes match 'OptionList' on PickScreen()` while the same test passes 12/12 locally. The context picker is the visible victim, but every caller of the modal shares the defect. Schedule the focus for after the next refresh instead. The retry is deliberately once-only so a screen that somehow never composes an option list degrades to "not focused" rather than looping forever. Test asserts the mount path schedules a retry instead of raising, reproducing the exact CI exception on an unmounted screen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/widgets/pick_screen.py | 15 ++++++++++++++- tests/ui/test_pick_screen.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 tests/ui/test_pick_screen.py diff --git a/src/korvid/ui/widgets/pick_screen.py b/src/korvid/ui/widgets/pick_screen.py index cb1d4d3d..e582e661 100644 --- a/src/korvid/ui/widgets/pick_screen.py +++ b/src/korvid/ui/widgets/pick_screen.py @@ -7,6 +7,7 @@ from textual.app import ComposeResult from textual.binding import Binding from textual.containers import Vertical, VerticalScroll +from textual.css.query import NoMatches from textual.screen import ModalScreen from textual.widgets import OptionList, Static @@ -62,7 +63,19 @@ def compose(self) -> ComposeResult: yield OptionList(*self._options) def on_mount(self) -> None: - option_list = self.query_one(OptionList) + self._focus_options() + + def _focus_options(self, *, retry: bool = True) -> None: + try: + option_list = self.query_one(OptionList) + except NoMatches: + # `on_mount` can fire before compose children are queryable; try + # again once the screen has refreshed rather than taking the app + # down. Only once, so a screen that somehow never composes an + # option list fails quietly instead of looping forever. + if retry: + self.call_after_refresh(self._focus_options, retry=False) + return option_list.highlighted = 0 option_list.focus() diff --git a/tests/ui/test_pick_screen.py b/tests/ui/test_pick_screen.py new file mode 100644 index 00000000..f6f62a85 --- /dev/null +++ b/tests/ui/test_pick_screen.py @@ -0,0 +1,28 @@ +"""`PickScreen` must survive being mounted before its options exist.""" + +from __future__ import annotations + +from korvid.ui.widgets.pick_screen import PickScreen + + +def test_mount_does_not_require_the_option_list_to_exist_yet() -> None: + """`on_mount` can fire before `compose` children are mounted. + + Textual does not guarantee that a screen's composed children are queryable + by the time `on_mount` runs. When the option list is not there yet, the + screen must schedule the focus instead of raising `NoMatches` and killing + the whole app (observed as an intermittent CI failure in + `tests/ui/test_ctx_switch.py`). + """ + screen = PickScreen("pick a context", ["ctx-a", "ctx-b"]) + scheduled: list[object] = [] + + def record(callback: object, *args: object, **kwargs: object) -> bool: + scheduled.append(callback) + return True + + screen.call_after_refresh = record # type: ignore[method-assign] # exercising the unmounted path without a running app + + screen.on_mount() + + assert scheduled, "focus should be retried once the option list is composed" From 70f5b51e3db741b00189cb03ad8585261b893c1d Mon Sep 17 00:00:00 2001 From: hellices Date: Fri, 7 Aug 2026 10:56:25 +0900 Subject: [PATCH 2/2] test(ui): prove the pick-modal focus retry only fires once Review caught that the previous test asserted a retry gets scheduled but never ran it, so the once-only guard the fix depends on was unverified: the test would have passed just as happily against an implementation that re-enqueued itself on every refresh forever. Invoke the captured callback and assert it schedules nothing further. Confirmed the assertion bites by flipping the implementation to retry=True, which fails it, then restoring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/ui/test_pick_screen.py | 42 ++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/tests/ui/test_pick_screen.py b/tests/ui/test_pick_screen.py index f6f62a85..3a43027c 100644 --- a/tests/ui/test_pick_screen.py +++ b/tests/ui/test_pick_screen.py @@ -2,9 +2,28 @@ from __future__ import annotations +from collections.abc import Callable +from typing import Any + from korvid.ui.widgets.pick_screen import PickScreen +def _capture_scheduled_calls(screen: PickScreen) -> list[Callable[..., Any]]: + """Record `call_after_refresh` callbacks instead of running them. + + The screen is never mounted in these tests, so there is no app to drive the + refresh; capturing the callback lets a test invoke it explicitly. + """ + scheduled: list[Callable[..., Any]] = [] + + def record(callback: Callable[..., Any], *args: Any, **kwargs: Any) -> bool: + scheduled.append(lambda: callback(*args, **kwargs)) + return True + + screen.call_after_refresh = record # type: ignore[method-assign] # exercising the unmounted path without a running app + return scheduled + + def test_mount_does_not_require_the_option_list_to_exist_yet() -> None: """`on_mount` can fire before `compose` children are mounted. @@ -15,14 +34,25 @@ def test_mount_does_not_require_the_option_list_to_exist_yet() -> None: `tests/ui/test_ctx_switch.py`). """ screen = PickScreen("pick a context", ["ctx-a", "ctx-b"]) - scheduled: list[object] = [] + scheduled = _capture_scheduled_calls(screen) - def record(callback: object, *args: object, **kwargs: object) -> bool: - scheduled.append(callback) - return True + screen.on_mount() - screen.call_after_refresh = record # type: ignore[method-assign] # exercising the unmounted path without a running app + assert scheduled, "focus should be retried once the option list is composed" + + +def test_the_deferred_focus_retry_does_not_reschedule_itself() -> None: + """A screen whose options never appear must stop, not spin. + The retry exists to survive one lost race, not to poll forever: if the + option list is still missing when the deferred callback runs, the screen + gives up quietly rather than queueing more work on every refresh. + """ + screen = PickScreen("pick a context", ["ctx-a", "ctx-b"]) + scheduled = _capture_scheduled_calls(screen) screen.on_mount() + retry = scheduled.pop() - assert scheduled, "focus should be retried once the option list is composed" + retry() + + assert not scheduled, "the retry must not queue another retry"