Fix multiselect parameter format for TOPP tool compatibility#323
Fix multiselect parameter format for TOPP tool compatibility#323t0mdavid-m merged 3 commits intomainfrom
Conversation
The multiselect widget returns a list, but TOPP tools expect newline-separated strings. Use a separate key for the multiselect widget and sync to the original key as a newline-separated string via on_change callback. https://claude.ai/code/session_016FpXUEwfPanXWu4CLrFLSA
Use default= parameter directly instead of manual session state initialization, more consistent with how selectbox uses index= parameter. https://claude.ai/code/session_016FpXUEwfPanXWu4CLrFLSA
📝 WalkthroughWalkthroughdisplay_TOPP_params now presents list-type TOPP parameters with a separate multiselect display key and an on_change callback that syncs the selected list into the main session_state key as a newline-joined string; ParameterManager.save_parameters skips keys ending with Changes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflow/StreamlitUI.py (1)
868-890: Sync main TOPP param key from display selection on every render, not just on initialization.The main key is only initialized when missing (line 889), which leaves a sync gap: if the key already exists from a prior parameter load and the current valid options have changed, the main key can retain stale/filtered-out values until the user changes the widget. This causes the saved TOPP parameter to diverge from the UI selection until user interaction.
Sync from
display_keyafter the widget is rendered to ensure the main key always reflects the actual current selection:🛠️ Proposed fix
cols[i].multiselect( name, options=sorted(p['valid_strings']), default=current_values, help=p["description"], key=display_key, on_change=on_multiselect_change, ) - # Ensure main key has string value for ParameterManager - if key not in st.session_state: - st.session_state[key] = "\n".join(current_values) + # Keep main key in sync with actual selection + selected = st.session_state.get(display_key, current_values) + st.session_state[key] = "\n".join(selected)
Skip keys ending in _display in save_parameters() to prevent multiselect display keys from being incorrectly saved to JSON or compared with INI values. https://claude.ai/code/session_016FpXUEwfPanXWu4CLrFLSA
The multiselect widget returns a list, but TOPP tools expect newline-separated strings. Use a separate key for the multiselect widget and sync to the original key as a newline-separated string via on_change callback.
https://claude.ai/code/session_016FpXUEwfPanXWu4CLrFLSA
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.