Skip to content

Fix selection error when changing ui.select options in an "input-value" handler - #6100

Merged
falkoschindler merged 2 commits into
mainfrom
select-input-value-order
Jun 12, 2026
Merged

Fix selection error when changing ui.select options in an "input-value" handler#6100
falkoschindler merged 2 commits into
mainfrom
select-input-value-order

Conversation

@falkoschindler

@falkoschindler falkoschindler commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Motivation

Fixes #4420.

When a ui.select with with_input=True changes its options inside an "input-value" handler (e.g. for server-side filtering of large option lists), selecting an option raises "list index out of range":

ui.select([], with_input=True) \
    .on('input-value', lambda e: e.sender.set_options([o for o in ['A1', 'A2'] if str(e.args) in o]))

Selecting an option fires two events nearly simultaneously: due to fill-input, the new input text triggers "input-value", and the selection itself triggers "update:model-value" with the option's index. QSelect emits "input-value" first, so the server refilters the options before processing the selection -- and the transmitted index no longer matches the new options list. Depending on how the options changed, this either raises an IndexError or silently resolves to the wrong value.

Implementation

select.js intercepts both events (declaring them in emits so the server-registered listeners no longer fall through to the inner QSelect) and forwards "input-value" one tick later -- unless an "update:model-value" event was emitted during the same tick. In other words: the "input-value" event accompanying a selection is suppressed entirely, so a selection never re-triggers the app's filtering, and "input-value" comes to mean "the user typed". For regular typing, nothing else is emitted in the same tick and the one-tick deferral is imperceptible.

The "update:model-value" interception has to re-emit synchronously rather than just observe: a plain template listener would shadow the server-registered fallthrough listener, because Vue resolves the camelCase handler key first and only calls that one.

Decisions and trade-offs (see also the discussion in #4420, #5682 and the review below):

  • A first iteration merely re-ordered the two events. The review showed that this converts the loud IndexError into a silent value reset whenever the refilter cannot re-match the fill-input text, which carries the label (most notably dict options filtered by key). Suppression eliminates both failure modes: the options the user selected from stay untouched until they actually type again.
  • No server-side index guard or label-based fallback: labels are not unique for dict options, and a stale-but-valid index is indistinguishable from a correct one. The residual race -- options changed by a timer or another user while a selection is in flight -- still fails loudly with IndexError rather than being half-masked.
  • Behavioral change: "input-value" no longer reaches handlers when an option is selected (or cleared, if QSelect resets the input text in the same tick) -- only on actual input changes. Known workarounds for Dynamic loading of ui.select options -> event handling error #4420 (sleeping in the handler, checking is_showing_popup) keep working; verified in a live browser along with the original example, the reviewer's dict reproduction, and a worst-case filter that removes the selected option.

Progress

  • The PR title is a short phrase starting with a verb like "Add ...", "Fix ...", "Update ...", "Remove ...", etc.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytest has been added.
  • Documentation is not necessary.
  • No breaking changes to the public API.

🤖 Generated with Claude Code

… is processed first

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@falkoschindler falkoschindler added bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review labels Jun 10, 2026
@falkoschindler falkoschindler added this to the 3.14 milestone Jun 10, 2026
@falkoschindler
falkoschindler requested a review from evnchn June 10, 2026 10:10
@evnchn

evnchn commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Posted by Claude Code on Evan's behalf.

Status note: a separate review pass flagged a potential footgun in this approach — we're working on a minimal reproduction to confirm or refute it. Holding our review until that lands (shortly); please hold off merging until then.

@evnchn evnchn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted by Claude Code on Evan's behalf.

Process transparency: a first review pass in Claude Web flagged a potential footgun but could not run code (full conversation); this session then verified every claim empirically against this branch and main — confirming the main finding, and refuting the mitigation that conversation proposed. All test code and outputs below.

TL;DR: The re-ordering fix is correct for the reported bug, and the interception mechanics check out (we verified the emits-interception down to Vue's emit-listener key matching — no double-fire, ordering preserved). But the new ordering converts a failure class from loud to silent: when an input-value handler refilters with anything that can't re-match the fill-input label — most notably dict options filtered by key, which we believe is the typical shape for the server-side-filtering use case this PR targets — the deferred event now lands after a successful selection, and set_options() silently resets the value to None, firing a second change event (A2 → None). On main the same code fails loudly with IndexError. The is_showing_popup workaround does not cover this case (confirmed: it deadlocks filtering bootstrap with empty initial options). Requesting changes so this trade-off is consciously decided and documented before merge — the approach itself seems right to us.

1. Confirmed: silent post-selection clobber (MRE, run on both branches)

One-token mutation of the PR's own test: dict options, filter on keys.

def test_dict_options_filtered_by_key_in_input_value_handler(screen: Screen):
    events = []

    @ui.page('/')
    def page():
        select = ui.select({}, with_input=True) \
            .on('input-value', lambda e: select.set_options({o: f'Option {o}' for o in ['A1', 'A2'] if str(e.args) in o}))
        select.on_value_change(lambda e: events.append(e.value))
        ui.label().bind_text_from(select, 'value', lambda v: f'value = {v!r}')

    screen.open('/')
    screen.find_by_tag('input').send_keys('A')
    screen.click('Option A2')
    screen.wait(1)
    print(f'CHANGE_EVENTS: {events}')
    screen.should_contain("value = 'A2'")

Walkthrough: type A → options become {'A1': 'Option A1', 'A2': 'Option A2'}. Click "Option A2" → update:model-value lands first (this PR's fix), value correctly becomes 'A2'. Then the deferred input-value arrives with e.args = 'Option A2' (fill-input writes the label), the key-based filter matches nothing, options collapse to {}, and ChoiceElement._update_options() (choice_element.py:47: self.value = before_value if before_value in self._values else None) resets the value.

Results (Screen tests, Chrome, this branch at 445afaa vs. main at 240db43):

branch CHANGE_EVENTS server log
this PR ['A2', None] — selection then silent clobber clean
main [] loud IndexError: list index out of range

So the PR's own design principle — "still fails loudly with IndexError rather than being half-masked" — holds for the timer/multi-user race, but this path goes the other way: a previously-loud failure becomes a silent wrong value.

2. Confirmed: is_showing_popup guard is NOT a viable mitigation here

The natural guard (only refilter while the popup is showing) deadlocks the bootstrap when initial options are empty — the shape of the PR's own example: with ui.select({}, ...), QSelect has no popup to show yet, so the first input-value arrives with is_showing_popup == False (debug-print confirmed), the refilter is skipped, options never populate, the popup never opens. Test fails with "Could not find 'Option A2'".

The PR body's claim that the known workarounds "keep working" was presumably verified against the original #4420 list-options shape; for the dict/key case above it doesn't transfer.

3. Verified working mitigation: retain the selected value in the refilter
def refilter(e):
    opts = {k: v for k, v in ALL.items() if str(e.args) in k or k == select.value}
    select.set_options(opts)

Passes on this branch with a single clean change event: CHANGE_EVENTS: ['A2']. If documentation is the chosen remedy, this is the pattern we'd suggest documenting (not the popup guard).

4. Nit: the "input-debounce" justification in the PR body is incorrect

select.py:81 sets 'input-debounce': 0 when with_input=True, so there is no debounce cushion; "well below the existing input-debounce" is wrong as stated. The deferral is still imperceptible (one tick), so this only needs a wording fix.

5. Possible directions (hedged — judgment calls, untested unless noted)
  • Minimum: document the silent-clobber path (+ the value-retention pattern from §3) and fix the §4 wording. The behavior change may well be acceptable once it's a documented, conscious trade-off.
  • Client-side idea (untested): in deferInputValue, skip the re-emit when an update:model-value was emitted during the same tick — selection-induced input-value events would vanish entirely instead of being re-ordered, eliminating both the loud and the silent variant. Typing behavior unchanged.
  • Framework-wide idea (separate scope): ChoiceElement._update_options() could warn when it silently nulls a previously-valid value — that silent reset is what makes this class of bug invisible.

…g them

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@falkoschindler

Copy link
Copy Markdown
Contributor Author

Thanks @evnchn for this outstanding review -- the silent-clobber finding is real and important.

I reproduced it with your MRE (now covered by the option_dict parametrization of test_changing_options_in_input_value_handler) and adopted the client-side direction you sketched in §5: forwardInputValue now waits one tick and skips the re-emit when an "update:model-value" event fired in the meantime. Selection-induced "input-value" events vanish entirely instead of being re-ordered, which eliminates both the loud and the silent variant -- including your dict/key case, where the selection now simply sticks. Even the worst case (a filter that removes the just-selected option) keeps the value now, since the refilter never runs on selection.

Two notes on the details:

  • §2: The bare popup guard indeed deadlocks, but the workaround from Dynamic loading of ui.select options -> event handling error #4420 also had a not e.sender.options bootstrap clause, which avoids the deadlock for empty initial options. Either way, with this PR the guard is no longer needed.
  • §4: Fair point, fixed in the PR body -- the deferral is one tick, there is no debounce cushion.

The trade-off to be conscious of now: "input-value" no longer fires on selection (or when QSelect rewrites the input in the same tick as a model-value change, e.g. clearing). I'd argue that's the semantics everyone expects from the event anyway, but it is a behavior change for anyone relying on the old quirk.

Verified in a live browser: the original #4420 example, your dict MRE, both published workarounds, and the destructive-filter case; full select test suite is green.

@evnchn evnchn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posted by Claude Code on Evan's behalf.

Approving — the suppression approach resolves everything from the review, verified empirically on the new head (9274f96):

  • The original dict/key MRE now passes with a single clean change event (['A2', None]['A2']): the selection sticks, no silent clobber, no loud error. Full select suite green locally (36/36, including the new option_dict parametrization).
  • Mechanics check out: both events intercepted via emits, forwardModelValue re-emits synchronously (server-side ordering and the loopback prop-store handling unchanged), and the suppression window is exactly the one tick where a selection-induced "input-value" can occur.
  • The documented trade-off ("input-value" no longer fires on selection or same-tick input rewrites) is the semantics users would expect from the event — agreed it's the right call, and it's now stated in the PR body.

Thanks for the quick turnaround and for adopting the MRE as a regression test — and good catch on the #4420 workaround's bootstrap clause (§2); you're right that it sidestepped the deadlock I flagged.

@falkoschindler
falkoschindler added this pull request to the merge queue Jun 12, 2026
Merged via the queue into main with commit 369c8b3 Jun 12, 2026
7 checks passed
@falkoschindler
falkoschindler deleted the select-input-value-order branch June 12, 2026 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dynamic loading of ui.select options -> event handling error

2 participants