Allow dynamic changes to st.select_slider options when key is provided#13696
Allow dynamic changes to st.select_slider options when key is provided#13696lukasmasuch merged 17 commits intodevelopfrom
st.select_slider options when key is provided#13696Conversation
…dentity Convert select_slider from index-based to string-based session state storage, enabling dynamic option updates while preserving valid selections. Aligns with patterns used by st.radio, st.selectbox, and st.multiselect. Co-Authored-By: Claude <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
lukasmasuch
added
change:feature
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request enables st.select_slider to support dynamic option changes when a key is provided, aligning its behavior with st.radio, st.selectbox, and st.multiselect. The implementation converts the widget from index-based to string-based session state storage using formatted option strings, allowing the widget to maintain its value when options change (as long as the selected option still exists in the new options list).
Changes:
- Converts select_slider from index-based to string-based value storage for dynamic option support
- Changes
key_as_main_identityfrom selective parameters toTruefor stable widget identity - Adds comprehensive validation logic to handle option changes gracefully (reset invalid values to defaults)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/streamlit/proto/Slider.proto | Adds raw_value field for string-based value storage in select_slider |
| lib/streamlit/elements/widgets/select_slider.py | Refactors SelectSliderSerde to use formatted strings instead of indices; adds value validation |
| lib/streamlit/elements/lib/options_selector_utils.py | Adds validate_and_sync_select_slider_value_with_options for validating values against current options |
| frontend/lib/src/components/widgets/Slider/Slider.tsx | Adds string/index conversion logic and rawValue proto handling for select_slider |
| frontend/lib/src/components/widgets/Slider/Slider.test.tsx | Adds TypeScript tests for string array value serialization and rawValue handling |
| lib/tests/streamlit/elements/select_slider_test.py | Adds Python unit tests for dynamic options, range sliders, and format_func; updates ID stability tests |
| e2e_playwright/st_select_slider.py | Adds test app code for dynamic options scenarios |
| e2e_playwright/st_select_slider_test.py | Adds E2E tests for dynamic option preservation and reset behavior |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
- Fix mypy errors in select_slider.py using cast instead of assertions - Update element_tree.py SelectSliderSerde usage for new signature - Improve serialize/deserialize to detect range values from data - Update validation function to handle ranges set via session state - Update enum coercion tests to reflect string-based serialization behavior Co-Authored-By: Claude <[email protected]>
- Fix serialize() and validation to accept both tuple and list for range values, matching the API signature which allows both types - Add negative assertions to E2E, Python, and TypeScript tests to catch regressions - Add test coverage for dynamic options with Enum values Co-Authored-By: Claude <[email protected]>
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0300%
💡 Consider adding more unit tests to maintain or improve coverage. |
lukasmasuch
changed the title
st.select_slider options when key is provided
📉 Python coverage change detectedThe Python unit test coverage has decreased by 0.0100%
✅ Coverage change is within normal range. Coverage by files
|
lukasmasuch
added
ai-review
github-actions
bot
removed
the
ai-review
SummaryThis PR enables Key changes:
Code QualityBackend (Python):
Frontend (TypeScript):
Proto changes:
Test CoverageUnit tests are comprehensive:
Frontend tests include:
E2E tests cover:
The tests follow best practices including negative assertions (e.g., verifying values are NOT preserved when they shouldn't be). Backwards CompatibilityThe implementation is backwards compatible:
Note on enum coercion behavior change: The tests in Security & RiskLow risk items identified:
Potential edge case: If two different options produce the same formatted string via RecommendationsNo blocking issues found. A few minor observations:
VerdictAPPROVED: The PR is well-implemented with comprehensive test coverage, follows established patterns in the codebase, maintains backwards compatibility, and properly addresses the feature request for dynamic option changes in This is an automated AI review using |
| # Use format_func comparison for all values. This correctly handles: | ||
| # - Custom objects without __eq__ (deepcopied instances) | ||
| # - Enum values (already from current class due to serde deserialization) | ||
| formatted_options_set = {format_func(o) for o in opt} | ||
| try: | ||
| formatted_value = format_func(current_value) | ||
| if formatted_value in formatted_options_set: | ||
| return current_value, False | ||
| except Exception: # noqa: S110 | ||
| pass # format_func failed - value is invalid, fall through to reset |
There was a problem hiding this comment.
I changed this recently for radio/selectbox, but I think we can keep this simpler and probably even more correct by just always using the formatted value here also for enum case (which is a super niche usecase anyways)
There was a problem hiding this comment.
Hmmm, OK, I guess we can see if someone reports an issue due to this change.
There was a problem hiding this comment.
yep, also ... I'm not even sure if the previous change was already rolled out
The Options prop tests were failing because they test select_slider functionality but didn't specify type: SliderProto.Type.SELECT_SLIDER. Without this, isSelectSlider() returns false and setDoubleArrayValue is called instead of setStringArrayValue. Co-Authored-By: Claude <[email protected]>
e2e_playwright/st_select_slider.py
Outdated
| # options are not yet supported for dynamic changes | ||
| # keeping it at the same value: | ||
| options=["red", "orange", "yellow", "green", "blue"], | ||
| # "green" is at index 0 here (shared with initial options) |
There was a problem hiding this comment.
I don't understand the (shared with initial option) part of this comment. Is this meant to describe how in this part of the test (initial state) the green option is at index 0? If so, I think this comment could be reworded to something like.
In the first part of the test, "green" is at index 0.
There was a problem hiding this comment.
Update the comment to clarify it a bit
e2e_playwright/st_select_slider.py
Outdated
| kwargs={"param": "initial kwarg param"}, | ||
| # options are not yet supported for dynamic changes | ||
| # keeping it at the same value: | ||
| # "green" is at index 3 here (shared with updated options) |
There was a problem hiding this comment.
Similar here, unless I am misunderstanding something, maybe this would be clearer?
After update, "green" is at index 3.
| if (isSelectSlider(element)) { | ||
| // For select_slider, get string values and convert to indices | ||
| const stringValues = widgetMgr.getStringArrayValue(element) | ||
| if (stringValues === undefined) { |
There was a problem hiding this comment.
[question] Is this an error case, or just part of the normal lifecycle?
There was a problem hiding this comment.
This is part of the normal lifecycle. It happens during initial render when no value has been stored in the widget manager yet. Added a clarifying comment.
- Update test comments to clearly describe option index positions - Add comment explaining normal lifecycle case in getStateFromWidgetMgr Co-Authored-By: Claude <[email protected]>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| def format_func(x): | ||
| return f"num_{x}" |
There was a problem hiding this comment.
The Python Guide requires adding typing annotations to every new function, method or class member. The function parameter 'x' in the format_func definition is missing a type annotation. Add a type annotation such as 'def format_func(x: Any) -> str:' to comply with the typing requirements.
| def format_func(x): | |
| return f"num_{x}" | |
| def format_func(x: Any) -> str: | |
| return f"num_{x}" |
Spotted by Graphite Agent (based on custom rule: Python Guide)
Is this helpful? React 👍 or 👎 to let us know.
| for i, s in enumerate(ui_value): | ||
| idx = self.formatted_option_to_index.get(s) | ||
| if idx is not None: | ||
| results.append((idx, self.options[idx])) |
There was a problem hiding this comment.
[nit] Maybe use .get() protectively in case the index is somehow out of range?
There was a problem hiding this comment.
Added a defensive check to make sure that it cannot go out of range
| results.append((default_idx, self.options[default_idx])) | ||
|
|
||
| if is_range and len(results) >= 2: | ||
| # Ensure start <= end by index |
There was a problem hiding this comment.
[nit] Maybe expand this and say, Ensure start <= end by returning deserialized range value in ascending order.
| assert "Selected: bravo" not in at.get("markdown")[-1].value | ||
|
|
||
| # Select "alpha" which exists in both option sets | ||
| at.select_slider[0].set_value("alpha").run() |
There was a problem hiding this comment.
[nit] This could be a bit better if there was another option shared between the arrays that could be selected here since it seems that alpha is the default as well.
sfc-gh-lwilby
left a comment
sfc-gh-lwilby
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments.
- Add type annotation to format_func parameter (x: Any) -> str - Add bounds check for index access in deserialize - Expand comment to clarify ascending order requirement - Improve test by using non-default shared option (delta) Co-Authored-By: Claude <[email protected]>
Describe your changes
Enable
st.select_sliderto support dynamic option changes when akeyis provided. Converts the widget from index-based to string-based session state storage, following the same pattern used byst.radio,st.selectbox, andst.multiselect. This allows the widget to maintain its value when options change, as long as the selected option still exists in the new options list.Changes:
raw_valuefield to Slider.proto for string-based value storageSelectSliderSerdeto use formatted option strings instead of indiceskey_as_main_identitytoTruefor stable widget identity with dynamic updatesstring_array_valueinstead ofdouble_array_valuefor session stateGitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.