Skip to content

fix: highlight the first provider when the login picker opens - #518

Open
ShreyanshVaibhaw wants to merge 2 commits into
huggingface:mainfrom
ShreyanshVaibhaw:fix/first-provider-not-selected-494
Open

fix: highlight the first provider when the login picker opens#518
ShreyanshVaibhaw wants to merge 2 commits into
huggingface:mainfrom
ShreyanshVaibhaw:fix/first-provider-not-selected-494

Conversation

@ShreyanshVaibhaw

Copy link
Copy Markdown
Contributor

Summary

Fixes #494.

Opening the login provider picker left nothing highlighted, so the first arrow-down landed on the second provider and openai could not be reached with the down key.

Root cause

LoginProviderPickerScreen._refresh_provider_list populated the list and set its index in the same synchronous pass:

provider_list.clear()
provider_list.extend([...])
provider_list.index = 0 if self.visible_providers else None

clear() and extend() are mount operations that complete asynchronously, so the index was assigned while the list was still empty and the reactive validated it straight back to None. Driving the real picker in a Textual harness:

open    : index=None  highlighted=no   classes=[[], [], []]
1x down : index=1  name=anthropic      <- first press skips the first provider

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 awaits clear()/extend() before setting its index, so the picker now matches existing practice in the file. on_mount and on_input_changed become async to await the refresh.

_select_visible_provider also 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:

open    : index=0  name=openai  highlighted=yes
1x down : index=1  name=anthropic
filtered: index=0  name=moonshotai

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_provider asserts 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_matches asserted the filtered list state after a single pilot.pause(), which no longer reliably settles now that the refresh awaits its mounts. It now uses await 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. ruff and mypy clean.

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

_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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First provider is not selected

1 participant