fix: highlight the first provider when the login picker opens - #518
Open
ShreyanshVaibhaw wants to merge 2 commits into
Open
fix: highlight the first provider when the login picker opens#518ShreyanshVaibhaw wants to merge 2 commits into
ShreyanshVaibhaw wants to merge 2 commits into
Conversation
_refresh_provider_list populated the ListView and set its index in the same synchronous pass. clear() and extend() complete asynchronously, so the index was assigned while the list was still empty and validated back to None. Nothing was highlighted when the picker opened, and a down key pressed before Textual's own mount cycle caught up skipped past the first provider - openai was effectively unreachable. Await the mounts before assigning the index, following the existing pattern in TreePickerScreen._toggle_tool_calls, and fall back to the first match when selecting from the search field. Fixes huggingface#494
The refresh now awaits its list mounts, so a single pilot.pause() no longer reliably settles the filtered list. Use the same wait_for_scheduled_animations() flush the sibling filtering test uses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #494.
Opening the login provider picker left nothing highlighted, so the first arrow-down landed on the second provider and
openaicould not be reached with the down key.Root cause
LoginProviderPickerScreen._refresh_provider_listpopulated the list and set its index in the same synchronous pass:clear()andextend()are mount operations that complete asynchronously, so the index was assigned while the list was still empty and the reactive validated it straight back toNone. Driving the real picker in a Textual harness:Textual's own mount cycle does eventually highlight row 0, which is why this reads as intermittent - but a user pressing a key as soon as the picker appears wins that race and skips the first entry.
Fix
Await the mounts before assigning the index. This is the pattern already used in
TreePickerScreen._toggle_tool_calls, which awaitsclear()/extend()before setting its index, so the picker now matches existing practice in the file.on_mountandon_input_changedbecome async to await the refresh._select_visible_provideralso falls back to the first match when no index is set, so submitting straight from the search field cannot land in a window where the highlight has not been applied.Verified after the change, stable across repeated runs:
I first also deferred a second index assignment via
call_after_refresh, then removed it after confirming the awaits alone are sufficient - the final change is the smaller one.Tests
test_tui_login_api_provider_picker_highlights_first_providerasserts the contract the fix establishes: when the refresh returns, the first provider is highlighted with no further event-loop turns, and one down press then moves to the second. It covers both the initial open and the filtered list, and I verified it fails on unfixed code and passes on fixed code.Worth noting: two earlier drafts of this test passed even without the fix, because
pilot.pause()gives Textual's mount cycle time to catch up and hides the race. Asserting on the refresh boundary is what makes it deterministic.One existing test needed a one-line update:
test_tui_login_api_provider_picker_handles_no_matchesasserted the filtered list state after a singlepilot.pause(), which no longer reliably settles now that the refresh awaits its mounts. It now usesawait pilot.wait_for_scheduled_animations(), the same flush its sibling filtering test already uses. Login tests pass 6/6 consecutive runs after the change (they were intermittently failing 1-in-4 before it).tests/test_tui_app.py: 339 passed.ruffandmypyclean.Note: 5 failures remain in that file in my environment, but they fail identically on unmodified
main- the Windows path-separator expectations from the closed #335 - and are unrelated to this change.Related
The same populate-then-index-immediately pattern appears in the session, prompt-template, tools, and skills pickers in
app.py, so they are likely affected by the same race. I kept this PR to the reported picker; happy to follow up on the rest if you would like them fixed the same way.🤖 Generated with Claude Code