Skip to content

Change default theme hash on app init #6729

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.

Sign up for GitHub

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

Jump to bottom
Merged
vdonato merged 1 commit into from
May 23, 2023
Merged

Change default theme hash on app init #6729

vdonato merged 1 commit into from
May 23, 2023
+6 −3

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented May 22, 2023 •
edited
Loading

In #6393, we added support for the host of an embedded Streamlit app to set the app's theme.
This currently conflicts with some of the theme caching code + custom theme code in the following
way:

  1. The Streamlit client initially loads, and its themeHash state variable is set to null
  2. The parent frame of the Streamlit app sets the theme of the app via a postMessage
  3. On the first script run for a browser tab, the client compares its current themeHash (null)
    with the hash used when it doesn't receive a custom theme from the server (the literal value
    ("hash_for_undefined_custom_theme").
  4. Because the two values in the previous step don't match, the client decides to reset the active
    theme to the system default (this handles the case where the developer removes a custom theme).

This can be fixed by simply initializing our themeHash to "hash_for_undefined_custom_theme" instead
of null, which will keep the behavior the same in all other cases but won't inadvertently revert themes set by
the parent frame.

@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label May 22, 2023
@vdonato
Copy link
Collaborator Author

vdonato commented May 22, 2023 •
edited
Loading

CC @sfc-gh-hkantuni @sfc-gh-wschmitt

This will probably go out a bit earlier than expected in 1.23 since we're pushing back that release by a week to get some other PRs in

sfc-gh-wschmitt
sfc-gh-wschmitt reviewed May 22, 2023
View reviewed changes
Copy link

@sfc-gh-wschmitt sfc-gh-wschmitt left a comment

Choose a reason for hiding this comment

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

@vdonato for president 🙌

Is there a way we can test this locally ahead of the release cycle? Potentially could we:

  1. start the frontend server dev server with no running backend
  2. window.post()
  3. then connect the backend?

@vdonato
Copy link
Collaborator Author

vdonato commented May 23, 2023 •
edited
Loading

Is there a way we can test this locally ahead of the release cycle?

Tested! Conveniently, https://github.com/streamlit/streamlit/blob/develop/frontend/hostframe.html was added last month for testing this, so that simplified this a good amount.

It was a bit trickier to test this than connecting to the dev server without a running backend since the client won't accept messages from its parent frame until after we get a response from the /_stcore/allowed-message-origins endpoint. The solution was to just add a ~5 second wait between getting a response from that endpoint and the websocket connecting to give me time to manually send a theme config via postMessage.

@vdonato vdonato merged commit 7a081a2 into develop May 23, 2023
@vdonato vdonato deleted the vdonato/default-theme-hash branch May 23, 2023 23:55
tconkling added a commit to tconkling/streamlit that referenced this pull request May 30, 2023
@tconkling
Merge branch 'develop' into tim/DevRequirementsNoProtobuf
31a39c0 
* develop: (22 commits)
  enzyme -> react-testing-library (Countdown, Modal, ProgressBar) (streamlit#6744)
  Remove console.log (streamlit#6753)
  Update `st.data_editor` and `st.dataframe` docstrings (streamlit#6752)
  Update `st.data_editor` session state format (streamlit#6711)
  Cypress flaky test fixes (streamlit#6743)
  Document integer size limit for number_input and slider (streamlit#6724)
  Change default theme hash on app init (streamlit#6729)
  Use .genericFonts instead of .fonts to fix bug with theme postMessage. (streamlit#6732)
  Improve startup performance by lazy loading some dependencies (streamlit#6531)
  Add support for Altair 5 (streamlit#6618)
  Update modals (streamlit#6688)
  Improve docstrings for `ttl` and `max_entries` (streamlit#6733)
  Improve `st.columns` docstring (streamlit#6727)
  Removed orphan line in dataframe docstring (streamlit#6734)
  Fix useIsOverflowing dependency array (streamlit#6731)
  Remove experimental from data editor (streamlit#6712)
  Clarify set_page_config docstring and exception message (streamlit#6594)
  Add a config option to disable warning for setting both a widget default and its key in session_state (streamlit#6640)
  Add column configuration API for `st.dataframe` and `st.data_editor` (streamlit#6598)
  Migrate datetime column formatting from date-fns to momentJS (streamlit#6702)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfc-gh-wschmitt sfc-gh-wschmitt sfc-gh-wschmitt left review comments

+2 more reviewers

@willhuang1997 willhuang1997 willhuang1997 approved these changes

@sfc-gh-wihuang sfc-gh-wihuang sfc-gh-wihuang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vdonato @willhuang1997 @sfc-gh-wschmitt @sfc-gh-wihuang