Skip to content

Conversation

@sfc-gh-pchiu
Copy link
Contributor

@sfc-gh-pchiu sfc-gh-pchiu commented Sep 16, 2024

Describe your changes

This PR addresses 2 issues with layout in both MPA v1 and v2 --

  • default page layout (centered) is not respected when navigating from a wide layout page
  • when a page's layout is dynamic, e.g. changeable based on user interaction, navigating back to that page does not preserve the last set layout

The proposed changes now preserve the layouts for each page in an MPA, by storing the mapping of page script hash <> layout in React state on page change. The layout of the page to be rendered will be looked up in every new session handling, defaulting to centered.

Demo: https://multipage-layout-fix.streamlit.app/

GitHub Issue Link

#6237

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-pchiu sfc-gh-pchiu added security-assessment-completed Security assessment has been completed for PR change:bugfix PR contains bug fix implementation impact:users PR changes affect end users labels Sep 18, 2024
@sfc-gh-pchiu sfc-gh-pchiu marked this pull request as ready for review October 3, 2024 01:57
@sfc-gh-pchiu sfc-gh-pchiu requested a review from kmcgrady October 3, 2024 01:57
@sfc-gh-pchiu sfc-gh-pchiu marked this pull request as draft October 3, 2024 01:58
@sfc-gh-pchiu
Copy link
Contributor Author

back to draft since I still need to add mpa v1 tests

@sfc-gh-pchiu sfc-gh-pchiu marked this pull request as ready for review October 3, 2024 21:42
Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@sfc-gh-pchiu sfc-gh-pchiu merged commit f1b4a11 into develop Oct 4, 2024
sfc-gh-pchiu added a commit that referenced this pull request Oct 30, 2024
…t in User Settings (#9723)

## Describe your changes

Update:
In this update, we revamped the `preferredLayouts` state in `App.tsx`
(previously `pageLayouts`) and fixed a number of issues with
1. apps flickering (when not calling `set_page_config`) 
2. apps having fragments rerunning
3. wide mode in User Settings

Here's a breakdown of the implementation by functions/files:
1. `handlePageConfigChanged` & `saveSetting`: `set_page_config` and User
Settings panel are the 2 only places that could change the layout. As a
result, we save the layout value to `preferredLayouts` in these 2
handlers
2. `handleNewSession` (not changed): For every rerun, we apply
previously saved `preferredLayouts` of the corresponding page. If none
matches, apply `CENTERED`
3. `handleNavigation`: If we get a new page script hash from navigation
which has never been initialized in `preferredLayouts`, we carry over
the layout from previous page for the new one.

--

~~We are seeing an unexpected behavior such that when an MPA page
containing fragments reruns, the page layout would change to centered.
This is an [issue reported in our
forum](https://discuss.streamlit.io/t/bad-setin-index-issue-with-using-fragments-that-do-not-render-anything-within-multipage-app/82675/9?u=raethlein)
flagged by @raethlein. Upon investigation, we have identified that the
fix in #9479 introduced this behavior change.~~

~~The root cause is that after a triggered rerun the page script hash
changes, causing the `pageLayouts` object state not recognizing the new
hash value and assigning the default/centered layout.
Our proposed fix here is to only perform layout update when there is no
fragment running.~~

~~I also added new e2e test cases with fragments to verify the fix.~~

## GitHub Issue Link (if applicable)
[Post in discuss
forum](https://discuss.streamlit.io/t/bad-setin-index-issue-with-using-fragments-that-do-not-render-anything-within-multipage-app/82675?u=raethlein)

## Testing Plan

- E2E Tests: new test case in MPAv2 that mocks fragment rerunning

### Manually testing app reported in forum
#### Before


https://github.com/user-attachments/assets/41c3f629-5f69-42c3-9e5f-e0fec9d1bff3

#### After


https://github.com/user-attachments/assets/77de515e-2e89-47d4-b49d-c03126d61f41


---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
kmcgrady added a commit that referenced this pull request Oct 30, 2024
edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025
## Describe your changes
This PR addresses 2 issues with layout in both MPA v1 and v2 --
* default page layout (centered) is not respected when navigating *from*
a wide layout page
* when a page's layout is dynamic, e.g. changeable based on user
interaction, navigating back to that page does not preserve the last set
layout

The proposed changes now preserve the layouts for each page in an MPA,
by storing the mapping of `page script hash <> layout` in React state on
page change. The layout of the page to be rendered will be looked up in
every new session handling, defaulting to centered.


Demo: https://multipage-layout-fix.streamlit.app/

## GitHub Issue Link
streamlit#6237

## Testing Plan

- Explanation of why no additional tests are needed
- Unit Tests (JS and/or Python)
- E2E Tests
- Any manual testing needed?

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025
…t in User Settings (streamlit#9723)

## Describe your changes

Update:
In this update, we revamped the `preferredLayouts` state in `App.tsx`
(previously `pageLayouts`) and fixed a number of issues with
1. apps flickering (when not calling `set_page_config`) 
2. apps having fragments rerunning
3. wide mode in User Settings

Here's a breakdown of the implementation by functions/files:
1. `handlePageConfigChanged` & `saveSetting`: `set_page_config` and User
Settings panel are the 2 only places that could change the layout. As a
result, we save the layout value to `preferredLayouts` in these 2
handlers
2. `handleNewSession` (not changed): For every rerun, we apply
previously saved `preferredLayouts` of the corresponding page. If none
matches, apply `CENTERED`
3. `handleNavigation`: If we get a new page script hash from navigation
which has never been initialized in `preferredLayouts`, we carry over
the layout from previous page for the new one.

--

~~We are seeing an unexpected behavior such that when an MPA page
containing fragments reruns, the page layout would change to centered.
This is an [issue reported in our
forum](https://discuss.streamlit.io/t/bad-setin-index-issue-with-using-fragments-that-do-not-render-anything-within-multipage-app/82675/9?u=raethlein)
flagged by @raethlein. Upon investigation, we have identified that the
fix in streamlit#9479 introduced this behavior change.~~

~~The root cause is that after a triggered rerun the page script hash
changes, causing the `pageLayouts` object state not recognizing the new
hash value and assigning the default/centered layout.
Our proposed fix here is to only perform layout update when there is no
fragment running.~~

~~I also added new e2e test cases with fragments to verify the fix.~~

## GitHub Issue Link (if applicable)
[Post in discuss
forum](https://discuss.streamlit.io/t/bad-setin-index-issue-with-using-fragments-that-do-not-render-anything-within-multipage-app/82675?u=raethlein)

## Testing Plan

- E2E Tests: new test case in MPAv2 that mocks fragment rerunning

### Manually testing app reported in forum
#### Before


https://github.com/user-attachments/assets/41c3f629-5f69-42c3-9e5f-e0fec9d1bff3

#### After


https://github.com/user-attachments/assets/77de515e-2e89-47d4-b49d-c03126d61f41


---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
@sfc-gh-pchiu sfc-gh-pchiu deleted the fix/mpa-page-layout branch March 10, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:bugfix PR contains bug fix implementation impact:users PR changes affect end users security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants