Fix selection error when changing ui.select options in an "input-value" handler - #6100
Conversation
… is processed first Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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
left a comment
There was a problem hiding this comment.
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 anupdate:model-valuewas emitted during the same tick — selection-inducedinput-valueevents 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>
|
Thanks @evnchn for this outstanding review -- the silent-clobber finding is real and important. I reproduced it with your MRE (now covered by the Two notes on the details:
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
left a comment
There was a problem hiding this comment.
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 newoption_dictparametrization). - Mechanics check out: both events intercepted via
emits,forwardModelValuere-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.
Motivation
Fixes #4420.
When a
ui.selectwithwith_input=Truechanges 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":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 anIndexErroror silently resolves to the wrong value.Implementation
select.js intercepts both events (declaring them in
emitsso 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):
IndexErrorinto 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.IndexErrorrather than being half-masked.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
🤖 Generated with Claude Code