fix(ui): stop the pick modal dying when it mounts before its options - #214
Conversation
`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>
There was a problem hiding this comment.
Pull request overview
Prevents intermittent PickScreen startup failures when composed children are not yet queryable.
Changes:
- Defers option-list focus until after refresh when necessary.
- Adds regression coverage for the unmounted-screen race.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/korvid/ui/widgets/pick_screen.py |
Adds a bounded focus retry. |
tests/ui/test_pick_screen.py |
Covers delayed option-list composition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/ui/test_pick_screen.py:28
- The test never executes the queued retry, so it would still pass if the callback were scheduled with
retry=Trueand kept re-enqueueing forever while theOptionListwas absent. Exercise the captured callback and assert that it schedules nothing else to cover the intended once-only safety behavior.
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
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>
|
Addressed the suppressed finding in You were right, and it mattered more than "suppressed" suggests: the commit message claimed the once-only retry as the safety property that keeps a never-composing screen from spinning, and the test did not check it at all. It would have passed against an implementation that re-enqueued itself on every refresh forever — exactly the bug the guard exists to prevent.
Full gate: 4307 passed, 21 skipped, ruff / mypy / tach clean. |
![my-reviewer-agent[bot]](https://avatars.githubusercontent.com/in/4343504?s=60&v=4){kind=small}
There was a problem hiding this comment.
자동 리뷰 결과
on_mount() 시점에 OptionList가 아직 query tree에 없을 수 있는 경합을 call_after_refresh 1회 재시도로 흡수하고, 재시도가 무한 반복되지 않음을 테스트한 방향은 타당합니다. 변경 범위도 작고 기존 정상 경로는 그대로 유지됩니다.
권고사항
src/korvid/ui/widgets/pick_screen.py:71-78— 두 번째 조회도 실패하면 조용히 반환하므로, 동일 문제가 재발했을 때 원인 추적이 어렵습니다. 최소한 debug/warning 로그를 남기면 진단성이 좋아집니다. 병합을 막을 사유는 아닙니다.- 현재 테스트는 미마운트 객체에서 예약 콜백 동작을 검증합니다. 후속으로 실제
App.run_test()에서 모달을 띄워OptionList가 focus되고 첫 항목이 highlight되는 성공 경로를 추가하면 Textual lifecycle 회귀를 더 직접적으로 잡을 수 있습니다.
버그·보안·성능·오류 처리 관점에서 병합을 막을 문제는 확인하지 못했습니다.
Verdict: COMMENT
test (3.12)went red onmainat 00c4cfd with:That is not a test bug and not a regression from the commit that exposed it.
PickScreen.on_mounthas calledquery_one(OptionList)unconditionally since #6, and Textual does not promise that a screen's composed children are queryable by the timeon_mountruns. Lose that race and the query raises, which kills the app while the modal is opening. The context picker is just the caller that happened to lose it;:transfer, the helm action pickers and every otherPickScreenuser share the defect.It only shows up under load, which is why CI catches it and laptops do not — the failing test passes 12/12 locally on the same commit.
Fix: schedule the focus for after the next refresh instead of assuming the option list is already there. The retry is once-only on purpose, so a screen that somehow never composes an option list degrades to "not focused" rather than spinning forever.
Test:
tests/ui/test_pick_screen.pydriveson_mounton an unmounted screen, which reproduces the exact CI exception, and asserts it schedules a retry instead of raising. Confirmed RED before the fix (sameNoMatchesmessage), GREEN after.Full gate: 4306 passed, 21 skipped, ruff / mypy / tach clean. The 188 tests across the picker-using suites (
test_ctx_switch,test_write_ops,test_transfer_picker,test_helm_actions) still pass, so selection and focus behaviour is unchanged.