-
Notifications
You must be signed in to change notification settings - Fork 4k
Update FormsContext
#12788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update FormsContext
#12788
Conversation
✅ 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!
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
bd2f963 to
8f45c0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves the consumption of formsData from Block.tsx down to the Form component, improving rendering performance by ensuring only Form components re-render when form data changes. It also removes addScriptFinishedHandler and removeScriptFinishedHandler from LibContext since these are only used in AppView. Additionally, the renderWithContexts test utility is enhanced to support updating context values during re-renders.
Key Changes:
- Moved
formsDataconsumption fromBlock.tsxtoForm.tsxfor better performance - Removed script finished handler methods from
LibContext - Enhanced
renderWithContextsto return arerenderWithContextsfunction for more flexible testing
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
frontend/lib/src/test_util.tsx |
Added RenderWithContextsResult interface and rerenderWithContexts function to support context updates during re-renders |
frontend/lib/src/components/widgets/Form/Form.tsx |
Moved formsData and scriptRunState consumption into Form component, removing props dependencies |
frontend/lib/src/components/widgets/Form/Form.test.tsx |
Updated tests to use new context-based approach with rerenderWithContexts |
frontend/lib/src/hooks/useWidgetManagerElementState.test.tsx |
Updated test to use renderWithContexts and provide formsData via context |
frontend/lib/src/components/core/Block/Block.tsx |
Removed formsData consumption and form-related prop calculations |
frontend/lib/src/components/core/LibContext.tsx |
Removed addScriptFinishedHandler and removeScriptFinishedHandler from context interface |
frontend/lib/src/components/core/FormsContext.tsx |
Updated documentation comments and fixed typo |
frontend/app/src/components/StreamlitContextProvider.tsx |
Removed script finished handler props from provider |
frontend/app/src/components/AppView/AppView.tsx |
Added script finished handlers as direct props instead of from context |
frontend/app/src/components/AppView/AppView.test.tsx |
Added mock handlers to test props |
frontend/app/src/App.tsx |
Moved script finished handlers from context to AppView props |
5c4a28d to
91407b6
Compare
91407b6 to
f1c3295
Compare
sfc-gh-bnisco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's a merge conflict with Block, but should be good to go afterwards!
f1c3295 to
f1903ca
Compare
f1903ca to
751c098
Compare

Describe your changes
Part 1 of
StreamlitContextProviderupdatesformsDatafromBlock.tsxrenderWithContextfunction to return arerenderWithContextsfunction to allow for changes in context values on rerender for more testing flexibilityRemoves
addScriptFinishedHandler&removeScriptFinishedHandlerfrom context as these are used one level down inAppViewcomponent.Testing Plan