[feat] Add default param to st.tabs#12313
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
sfc-gh-bnisco
added
security-assessment-completed
change:feature
✅ PR preview is ready!
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a default parameter to the st.tabs function, allowing users to specify which tab should be initially selected. The implementation includes validation for duplicate tab names and ensures the correct tab index is used when duplicate labels exist.
Key Changes
- Added
defaultparameter tost.tabsAPI with validation logic - Updated frontend components to use the default tab index from the backend
- Refactored layout handling to properly extract layout-relevant fields from block submessages
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proto/streamlit/proto/Block.proto | Added default_tab_index field to TabContainer message |
| lib/streamlit/elements/layouts.py | Implemented default parameter validation and index calculation |
| lib/tests/streamlit/elements/layouts_test.py | Added comprehensive test coverage for default tab functionality |
| frontend/lib/src/components/elements/Tabs/Tabs.tsx | Updated React component to use default tab index from backend |
| frontend/lib/src/components/elements/Tabs/Tabs.test.tsx | Added frontend tests for default tab behavior |
| frontend/lib/src/components/core/Block/Block.tsx | Refactored layout field extraction to handle TabContainer blocks properly |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| ) | ||
| const [activeTabKey, setActiveTabKey] = useState<React.Key>(defaultTabIndex) | ||
| const [activeTabName, setActiveTabName] = useState<string>(() => { | ||
| const tab = node.children[defaultTabIndex] as BlockNode |
There was a problem hiding this comment.
Array bounds error: node.children[defaultTabIndex] can cause runtime crash if defaultTabIndex is >= node.children.length. The defaultTabIndex comes from the backend and could be out of bounds if tabs are modified between renders or if there's a mismatch between frontend and backend state. Add bounds checking: const tab = node.children[defaultTabIndex] ?? node.children[0]
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Is it guaranteed that node.children is always fully loaded with the entire list of tabs here or do we need a fallback here?
There was a problem hiding this comment.
I don't know if it's guaranteed that node.children will always be fully loaded. With that said, we have a fallback in the following line, so this should be safe.
There was a problem hiding this comment.
But if the defaultTabIndex is higher than the number of children (e.g. on initial load), wouldn't this cause an array bounds error here?
There was a problem hiding this comment.
Good question, but it will return undefined, not throw an error.
| ) | ||
| const [activeTabKey, setActiveTabKey] = useState<React.Key>(defaultTabIndex) | ||
| const [activeTabName, setActiveTabName] = useState<string>(() => { | ||
| const tab = node.children[defaultTabIndex] as BlockNode |
There was a problem hiding this comment.
Is it guaranteed that node.children is always fully loaded with the entire list of tabs here or do we need a fallback here?
Describe your changes
defaultpram tost.tabsGitHub Issue Link (if applicable)
Fixes #10910
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.