Allow time for widget state to update #8697
Closed
Conversation
This file contains 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
## Describe your changes Combine all logic of the navigation (page url updates, mpa management), and put it in a class (V1AppNavigation). We create a layer above it that essentially pulls from the V1AppNavigation as an overengineering effort to set up for AppNavigation V2 ## Testing Plan - Original tests should pass - unit tests for AppNavigation. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes Migrates pages are primarily managed in the `source_util` module to a `PagesManager` class. Because V2 can have a dynamic set of pages per session, the Pages Manager instance will live on the `AppSession`. For V1, we will leverage static variables/methods for page watching. ## Testing Plan - Original Unit tests are applied (with patches adjusted) - Unit tests are applied for the PagesManager component --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes From #8639, @raethlein made a good claim about the lock as well as some types to make things clear, so I addressed those here. ## Testing Plan - Type checks should pass - Existing tests should pass with lock behavior (we don't usually manage concurrency testing at the moment) --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes MPA v2 will introduce the concept of an active script hash where the active script is the current script running. This change does the following: - Adds the concept of Active Script Hash to the Pages Manager. By default, it returns the current page hash and doesn't set anything. - All widgets will leverage the Active Script Hash in calculating their id. We do this so that widgets can be associated with the script (and avoid duplicate issues where possible). - We will provide the active script hash as part of the forward message metadata. That way, we can associate the active script hash with every call (this is especially important for `Delta` messages. - Frontend will store the active script hash in the nodes ## Testing Plan - Added a python unit test for the active script hash setting in the forward message. - Open to ideas on how to test the widget id updates, but I think it's not quite feasible. - Frontend tests storage of active script hash in nodes --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes MPA v2 requires to understand which code is associated with the main script (or common script) or not. We can send the main script hash in the NewSession ## Testing Plan - Updated unit tests to include the main script hash - Added a test that verifies the main script hash is set correctly --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes Confirmed that the properties associated with the `updateReport` call are not needed. ## Testing Plan - Added a unit test to ensure the value is sent on new session --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
#8658) ## Describe your changes MPA v2 will present different implementations from previous versions. We will adopt a strategy design pattern in order to separate implementation from MPA v1 or v2. ## Testing Plan - Original unit tests from this feature branch should still apply --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes This change includes `st.Page` and `st.navigation` commands bringing us closer to the finish point for MPA v2. ## Testing Plan - Added associated unit tests - E2E Tests will be needed to test MPA end-to-end, but they will not be included in this PR to the feature branch. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes There was a good recommendation to move position to an enum. Let's move it before committing the feature. ## Testing Plan - Updated Unit Tests --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes Others (Notebooks) uses `AppRoot.empty()`. To make things easier, I make the `mainScriptHash` keyword only. ## Testing Plan - Existing tests should be fine enough --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
## Describe your changes Title says it all. Ensure `st.Page` is an argument to `switch_page` and `page_link` ## Testing Plan - Unit Tests for `page_link` - E2E Tests will be added for `switch_page` --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
vdonato
reviewed
May 16, 2024
There was a problem hiding this comment.
How feasible would it be to try to figure out precisely what the race condition is rather than adding more of these time.sleep() calls?
I didn't realize that we started doing this in some places in the codebase, but I'd strongly prefer to figure out the underlying race and fix the bug so that we can get rid of all of these calls rather than add any more of them
|
We might be able to try not setting it and see if it works. I'll do some more testing. |
|
@vdonato @kmcgrady I have filed a discussion item for our next team meeting about this. I would suggest to not block this MPA project on it and rather make it a dedicated project to resolve this (high priority, so that this pattern does not continue to evolve), as it seems to touch multiple other places (if it is the same error). What do you think? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe your changes
We have found the app reacting too quickly on page change can lead to less desirable widget state behavior. Let's add a small sleep (similar to other situations)
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.