Merged
Conversation
📝 WalkthroughWalkthroughAdded reactive rendering and on_change callback support to StreamlitUI input APIs, refactored widget rendering into fragmented and direct implementation paths, and threaded on_change/reactive parameters through all internal widget invocation sites. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client/User
participant UI as StreamlitUI
participant Frag as Fragment Renderer
participant Impl as Direct Impl (parent context)
participant Ext as External Runner (TOPP/Python)
User->>UI: interact with widget (triggers event)
alt reactive == True
UI->>Impl: call _*_impl with on_change
Impl->>Ext: run handler / rerun parent
Ext-->>Impl: result
Impl-->>UI: update state
UI-->>User: updated UI
else reactive == False
UI->>Frag: call _*_fragmented with on_change
Frag->>Ext: run handler inside fragment
Ext-->>Frag: fragment result
Frag-->>UI: patch fragment
UI-->>User: updated fragment
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
354-368: AddOptionalto type hint foron_changeparameter and document it in the docstring.The
on_changeparameter at line 357 has an implicit Optional type (Callable = None), which violates the ruff RUF013 rule enabled in this project. Additionally,on_changeis missing from the method's docstring despite being a public parameter that affects widget behavior (as seen in calls tost.text_input(),st.checkbox(), etc.).Proposed fix
-from typing import Any, Union, List, Literal, Callable +from typing import Any, Union, List, Literal, Callable, Optional @@ - on_change: Callable = None, + on_change: Optional[Callable] = None, @@ display_file_path (bool, optional): Whether to display the full file path for file options. + on_change (Optional[Callable], optional): Callback function triggered when the widget + value changes. The callback receives no arguments. Default is None (no callback). reactive (bool, optional): If True, widget changes trigger the parent
🤖 Fix all issues with AI agents
In `@src/workflow/StreamlitUI.py`:
- Around line 529-586: The auto branch double‑prefixes keys when it recursively
calls _input_widget_impl (because _input_widget_impl already applies
param_prefix), so stop passing an already‑prefixed key: compute or use the
raw/unprefixed key when calling _input_widget_impl (e.g., strip
self.param_prefix or add an argument like already_prefixed=False) so
param_prefix is applied only once; also replace the equality check "value ==
None" with "value is None" in the auto branch. Reference: widget_type == "auto"
block and the _input_widget_impl method/param_prefix handling.
Comment on lines
529
to
+586
| elif widget_type == "auto": | ||
| # Auto-determine widget type based on value | ||
| if isinstance(value, bool): | ||
| st.checkbox(name, value=value, key=key, help=help) | ||
| st.checkbox(name, value=value, key=key, help=help, on_change=on_change) | ||
| elif isinstance(value, (int, float)): | ||
| self.input_widget( | ||
| self._input_widget_impl( | ||
| key, | ||
| value, | ||
| widget_type="number", | ||
| name=name, | ||
| help=help, | ||
| widget_type="number", | ||
| options=None, | ||
| min_value=min_value, | ||
| max_value=max_value, | ||
| step_size=step_size, | ||
| help=help, | ||
| display_file_path=False, | ||
| on_change=on_change, | ||
| ) | ||
| elif (isinstance(value, str) or value == None) and options is not None: | ||
| self.input_widget( | ||
| self._input_widget_impl( | ||
| key, | ||
| value, | ||
| widget_type="selectbox", | ||
| name=name, | ||
| options=options, | ||
| help=help, | ||
| widget_type="selectbox", | ||
| options=options, | ||
| min_value=None, | ||
| max_value=None, | ||
| step_size=1, | ||
| display_file_path=False, | ||
| on_change=on_change, | ||
| ) | ||
| elif isinstance(value, list) and options is not None: | ||
| self.input_widget( | ||
| self._input_widget_impl( | ||
| key, | ||
| value, | ||
| widget_type="multiselect", | ||
| name=name, | ||
| options=options, | ||
| help=help, | ||
| widget_type="multiselect", | ||
| options=options, | ||
| min_value=None, | ||
| max_value=None, | ||
| step_size=1, | ||
| display_file_path=False, | ||
| on_change=on_change, | ||
| ) | ||
| elif isinstance(value, bool): | ||
| self.input_widget( | ||
| key, value, widget_type="checkbox", name=name, help=help | ||
| self._input_widget_impl( | ||
| key, value, name=name, help=help, widget_type="checkbox", | ||
| options=None, min_value=None, max_value=None, step_size=1, | ||
| display_file_path=False, on_change=on_change | ||
| ) | ||
| else: | ||
| self.input_widget(key, value, widget_type="text", name=name, help=help) | ||
| self._input_widget_impl( | ||
| key, value, name=name, help=help, widget_type="text", | ||
| options=None, min_value=None, max_value=None, step_size=1, | ||
| display_file_path=False, on_change=on_change | ||
| ) |
There was a problem hiding this comment.
Avoid double‑prefixing keys in auto mode (breaks session_state/param mapping).
_input_widget_impl prefixes key (Line 443), but the auto-mode recursion passes that prefixed key back into _input_widget_impl, resulting in param_prefix being applied twice (e.g., param__param__foo). This can desync widget state and parameter persistence. Also switch value == None to is None.
🐛 Proposed fix
- if key in self.params.keys():
- value = self.params[key]
+ raw_key = key
+ if raw_key in self.params.keys():
+ value = self.params[raw_key]
@@
- key = f"{self.parameter_manager.param_prefix}{key}"
+ key = f"{self.parameter_manager.param_prefix}{raw_key}"
@@
- elif isinstance(value, (int, float)):
+ elif isinstance(value, (int, float)):
self._input_widget_impl(
- key,
+ raw_key,
value,
@@
- elif (isinstance(value, str) or value == None) and options is not None:
+ elif (isinstance(value, str) or value is None) and options is not None:
self._input_widget_impl(
- key,
+ raw_key,
value,
@@
- elif isinstance(value, list) and options is not None:
+ elif isinstance(value, list) and options is not None:
self._input_widget_impl(
- key,
+ raw_key,
value,
@@
- elif isinstance(value, bool):
+ elif isinstance(value, bool):
self._input_widget_impl(
- key, value, name=name, help=help, widget_type="checkbox",
+ raw_key, value, name=name, help=help, widget_type="checkbox",
options=None, min_value=None, max_value=None, step_size=1,
display_file_path=False, on_change=on_change
)
else:
self._input_widget_impl(
- key, value, name=name, help=help, widget_type="text",
+ raw_key, value, name=name, help=help, widget_type="text",
options=None, min_value=None, max_value=None, step_size=1,
display_file_path=False, on_change=on_change
)🧰 Tools
🪛 Ruff (0.14.13)
547-547: Comparison to None should be cond is None
Replace with cond is None
(E711)
🤖 Prompt for AI Agents
In `@src/workflow/StreamlitUI.py` around lines 529 - 586, The auto branch
double‑prefixes keys when it recursively calls _input_widget_impl (because
_input_widget_impl already applies param_prefix), so stop passing an
already‑prefixed key: compute or use the raw/unprefixed key when calling
_input_widget_impl (e.g., strip self.param_prefix or add an argument like
already_prefixed=False) so param_prefix is applied only once; also replace the
equality check "value == None" with "value is None" in the auto branch.
Reference: widget_type == "auto" block and the _input_widget_impl
method/param_prefix handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/workflow/StreamlitUI.py`:
- Around line 382-383: The parameter annotation for on_change is implicitly
Optional (default None) and must be explicit per PEP 484: import Optional from
typing (consistent with existing Union-style imports) and change the function
signature parameter from on_change: Callable = None to on_change:
Optional[Callable] = None; update any other occurrences in StreamlitUI.py that
reference this parameter type if present to use Optional[Callable] as well.
♻️ Duplicate comments (1)
src/workflow/StreamlitUI.py (1)
448-602: Prevent double‑prefixing keys in auto mode.
After Line 459,keyis already prefixed. The auto branch recurses into_input_widget_implwith the prefixed key (Lines 550–602), so it gets prefixed again and can desyncsession_state/param mapping. Also, Line 563 should useis None.🐛 Proposed fix
- if key in self.params.keys(): - value = self.params[key] + raw_key = key + if raw_key in self.params.keys(): + value = self.params[raw_key] else: value = default @@ - key = f"{self.parameter_manager.param_prefix}{key}" + key = f"{self.parameter_manager.param_prefix}{raw_key}" @@ - elif (isinstance(value, str) or value == None) and options is not None: + elif (isinstance(value, str) or value is None) and options is not None: self._input_widget_impl( - key, + raw_key, value, @@ - elif isinstance(value, list) and options is not None: + elif isinstance(value, list) and options is not None: self._input_widget_impl( - key, + raw_key, value, @@ - elif isinstance(value, bool): + elif isinstance(value, bool): self._input_widget_impl( - key, value, name=name, help=help, widget_type="checkbox", + raw_key, value, name=name, help=help, widget_type="checkbox", options=None, min_value=None, max_value=None, step_size=1, display_file_path=False, on_change=on_change ) else: self._input_widget_impl( - key, value, name=name, help=help, widget_type="text", + raw_key, value, name=name, help=help, widget_type="text", options=None, min_value=None, max_value=None, step_size=1, display_file_path=False, on_change=on_change )
Comment on lines
+382
to
+383
| on_change: Callable = None, | ||
| reactive: bool = False, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Python version requirements in config files
echo "=== Searching for Python version specs ==="
fd -a 'pyproject.toml|setup.cfg|setup.py|.python-version' -t f | head -20
echo ""
echo "=== Checking pyproject.toml ==="
cat pyproject.toml 2>/dev/null | head -40
echo ""
echo "=== Checking setup.cfg ==="
cat setup.cfg 2>/dev/null | head -40
echo ""
echo "=== Checking setup.py ==="
cat setup.py 2>/dev/null | head -40Repository: OpenMS/streamlit-template
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Verify the actual code at lines 382-383
echo "=== Viewing StreamlitUI.py around line 382-383 ==="
sed -n '375,390p' src/workflow/StreamlitUI.pyRepository: OpenMS/streamlit-template
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Check for Callable import and Optional usage patterns in the file
echo "=== Checking imports at top of StreamlitUI.py ==="
head -50 src/workflow/StreamlitUI.py | grep -E "^from|^import"
echo ""
echo "=== Checking if Optional is imported or used ==="
rg "Optional\[|from typing|import.*Callable" src/workflow/StreamlitUI.py | head -20Repository: OpenMS/streamlit-template
Length of output: 597
Add explicit Optional type hint for on_change.
The parameter uses implicit Optional (default None without Optional[...] annotation), which violates PEP 484. Since the file uses Union syntax rather than the | operator, import Optional and update the annotation to Optional[Callable].
♻️ Proposed fix
-from typing import Any, Union, List, Literal, Callable
+from typing import Any, Union, List, Literal, Callable, Optional- on_change: Callable = None,
+ on_change: Optional[Callable] = None,🧰 Tools
🪛 Ruff (0.14.13)
382-382: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In `@src/workflow/StreamlitUI.py` around lines 382 - 383, The parameter annotation
for on_change is implicitly Optional (default None) and must be explicit per PEP
484: import Optional from typing (consistent with existing Union-style imports)
and change the function signature parameter from on_change: Callable = None to
on_change: Optional[Callable] = None; update any other occurrences in
StreamlitUI.py that reference this parameter type if present to use
Optional[Callable] as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.