InputTOPP: add reactive option and split impl/fragment for conditional UI#398
InputTOPP: add reactive option and split impl/fragment for conditional UI#398JKVISION1101 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesTOPP reactive rendering
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)
822-834: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing
flag_parametersin the method signature causes aNameError.The
flag_parametersargument is documented in the docstring and passed to the internal implementation methods on lines 865 and 871, but it is missing from the method signature here. Because it is evaluated unconditionally in the method body, every call toinput_TOPPwill crash with aNameError.🐛 Proposed fix
def input_TOPP( self, topp_tool_name: str, num_cols: int = 4, exclude_parameters: List[str] = [], include_parameters: List[str] = [], + flag_parameters: List[str] = [], display_tool_name: bool = True, display_subsections: bool = True, display_subsection_tabs: bool = False, custom_defaults: dict = {}, tool_instance_name: str = None, reactive: bool = False, ) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/StreamlitUI.py` around lines 822 - 834, Add the documented flag_parameters argument to the input_TOPP method signature, using the same type and default expected by its internal implementation calls. Keep the existing forwarding at the relevant internal method invocations so the parameter is defined for every input_TOPP call.Source: Linters/SAST tools
🧹 Nitpick comments (1)
src/workflow/StreamlitUI.py (1)
876-888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid using mutable defaults for arguments.
The newly added internal methods
_input_TOPP_fragmentedand_input_TOPP_impl(as well asinput_TOPP) use mutable data structures ([],{}) for argument defaults. This can lead to unexpected behavior if the lists or dicts are mutated, as the changes will persist across function calls.Consider replacing them with
Noneand initializing them inside the function body.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/StreamlitUI.py` around lines 876 - 888, Replace the mutable defaults in _input_TOPP_fragmented, _input_TOPP_impl, and input_TOPP with None, then initialize each list or dict inside the corresponding function before use. Preserve the existing defaults and behavior when callers omit these arguments.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workflow/StreamlitUI.py`:
- Around line 895-907: Implement handling for flag_parameters within
_input_TOPP_impl so each listed parameter is rendered and submitted as a
no-value CLI flag rather than requiring an input value. Preserve the existing
behavior for parameters not included in flag_parameters, and ensure the argument
is applied consistently through the function’s parameter-processing path.
---
Outside diff comments:
In `@src/workflow/StreamlitUI.py`:
- Around line 822-834: Add the documented flag_parameters argument to the
input_TOPP method signature, using the same type and default expected by its
internal implementation calls. Keep the existing forwarding at the relevant
internal method invocations so the parameter is defined for every input_TOPP
call.
---
Nitpick comments:
In `@src/workflow/StreamlitUI.py`:
- Around line 876-888: Replace the mutable defaults in _input_TOPP_fragmented,
_input_TOPP_impl, and input_TOPP with None, then initialize each list or dict
inside the corresponding function before use. Preserve the existing defaults and
behavior when callers omit these arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c50c6772-d29e-445c-90f2-1d9ecd6e5178
📒 Files selected for processing (1)
src/workflow/StreamlitUI.py
| def _input_TOPP_impl( | ||
| self, | ||
| topp_tool_name: str, | ||
| num_cols: int = 4, | ||
| exclude_parameters: List[str] = [], | ||
| include_parameters: List[str] = [], | ||
| flag_parameters: List[str] = [], | ||
| display_tool_name: bool = True, | ||
| display_subsections: bool = True, | ||
| display_subsection_tabs: bool = False, | ||
| custom_defaults: dict = {}, | ||
| tool_instance_name: str = None, | ||
| ) -> None: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing implementation for flag_parameters.
The flag_parameters argument is passed into _input_TOPP_impl, but it is never used within the function body. The documented behavior of marking parameters as no-value CLI flags is currently a no-op.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 899-899: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 900-900: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 901-901: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 905-905: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
[warning] 906-906: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow/StreamlitUI.py` around lines 895 - 907, Implement handling for
flag_parameters within _input_TOPP_impl so each listed parameter is rendered and
submitted as a no-value CLI flag rather than requiring an input value. Preserve
the existing behavior for parameters not included in flag_parameters, and ensure
the argument is applied consistently through the function’s parameter-processing
path.
1 participant
Summary: Adds a reactive parameter to input_TOPP and splits the implementation into _input_TOPP_impl and _input_TOPP_fragmented so callers can opt into immediate parent re-rendering when a parameter changes.
What changed: input_TOPP no longer decorated with @st.fragment; it accepts reactive: bool and delegates to either _input_TOPP_impl (direct rendering) or _input_TOPP_fragmented (fragmented wrapper).
+Also documents a new flag_parameters argument used to mark no-value TOPP flags.
Summary by CodeRabbit
New Features
Compatibility