Refactor: withHostCommunication -> HostCommunicationManager #6746

mayagbarnes · 2023-05-25T16:20:02Z

📚 Description

Refactor our withHostCommunication HOC into a HostCommunicationManager class in an effort to simplify the current code & future extension.

Tech Spec: link

What kind of change does this PR introduce?
- Refactoring

🧪 Testing Done

Added/Updated unit tests
Added/Updated e2e tests

frontend/src/lib/hostComm/HostCommunicationManager.tsx

mayagbarnes · 2023-05-30T17:57:48Z

Notes on outstanding/followup items:

Explored some avenues to convert our hostComm manual testing setup (hostframe.html) to automated e2e tests and have it working, but going to separate that into another mini project since it will require adjustments to allowedOrigins and want to run that by security
Noticed that the CLOSE_MODAL host message works on all the MainMenu items with the exception of the screencast modal - not a result of this PR, its also the case on develop - just want to confirm this isn't an intentional exception?

tconkling

This is really fantastic. Most of my feedback is just nitpicking, but there are a few correctness issues I want to make sure we nail before merging.

(Also I think it's probably worth having @vdonato and/or @lukasmasuch give a second review afterwards, to make sure I didn't miss anything important.)

frontend/src/lib/hostComm/HostCommunicationManager.tsx

frontend/src/lib/hostComm/HostCommunicationManager.test.tsx

frontend/src/app/App.test.tsx

frontend/src/lib/hostComm/HostCommunicationManager.tsx

+  /**
+   * Register a function to handle a message from the Host
+   */
+  public receiveHostMessage = (event: MessageEvent): void => {


tconkling

I noticed a few unimplemented test stubs that should be filled in before merging.

I also left some questions about some App.test.tsx tests that seem to have been removed, but it's quite likely they're made obsolete with these changes? Just want to verify.

(And I also noted a bunch of tests that were just rearranged in App.test.tsx - these are just notes to myself for future reviews, so that I don't go looking for them again. No need to respond to those ones!)

tconkling · 2023-05-31T23:19:10Z

frontend/src/app/App.test.tsx

-      currentPageScriptHash: "hash1",
-    })
-  })
+  it("plumbs appPages and currentPageScriptHash to the AppView component", () => {})


Where did this test go?

My mistake - added back 👍🏼

tconkling · 2023-05-31T23:19:50Z

frontend/src/app/App.test.tsx

    })
-  })

-  it("responds to page change requests", () => {


Is this covered elsewhere?

Yup! this test forces update to HostCom HOC state (same thing as receiving REQUEST_PAGE_CHANGE message from host) to test onPageChange firing. This logic has moved to HostCommMgr -> the parallel test is on line 165 of HostCommunicationManager.test.tsx here

tconkling · 2023-05-31T23:21:40Z

frontend/src/app/App.test.tsx

-    expect(onModalReset).toBeCalled()
-  })

-  it("changes scriptRunState and fires withHostCommunication callback when scriptStopRequested signal has been received", () => {


Is this test obsolete?

Correct, these are part of that loop pattern we eliminated and this callback firing is tested in HostCommunicationManager instead.

tconkling · 2023-05-31T23:23:21Z

frontend/src/app/App.test.tsx

+        onClose: () => {},
+      })

-  it("changes scriptRunState and fires withHostCommunication callback when scriptRerunRequested signal has been received", () => {


Is this test obsolete? (I think the answer is yes, just noting where I see a test removal so I don't accidentally re-review tests that we expect to be removed.)

Yup! Ditto to above

tconkling · 2023-05-31T23:23:46Z

frontend/src/app/App.test.tsx

+        "stopScript"
+      )

-  it("fires withHostCommunication callback when cacheClearRequested signal has been received", () => {


Verifying this is intended to be removed

tconkling · 2023-05-31T23:28:22Z

frontend/src/app/App.test.tsx

+        })
+      )

-  it("updates state.appPages when it receives a PagesChanged msg", () => {


(Tim note: test was moved)

tconkling · 2023-05-31T23:28:55Z

frontend/src/app/App.test.tsx

    expect(instance.clearAppState).not.toHaveBeenCalled()
  })

-  it("sets hideSidebarNav based on the server config option and host setting", () => {


(Tim note: test was moved)

tconkling · 2023-05-31T23:29:33Z

frontend/src/app/App.test.tsx

      expect(wrapper.find("DeployButton")).toHaveLength(0)
    })
-
-    it("button should be hidden for cloud environment", () => {


Where did this test go?

This test was moved to line 591 - title Deploy button should be hidden for cloud environment

tconkling · 2023-05-31T23:30:18Z

frontend/src/app/App.test.tsx

-
-    expect(wrapper.instance().openClearCacheDialog).not.toBeCalled()
-  })
+  it("Tests dev menu shortcuts cannot be accessed as a viewer", () => {})


Missing test?

Added back!

frontend/src/lib/hostComm/HostCommunicationManager.tsx

tconkling

I noticed a few unimplemented test stubs that should be filled in before merging.

I also left some questions about some App.test.tsx tests that seem to have been removed, but it's quite likely they're made obsolete with these changes? Just want to verify.

(And I also noted a bunch of tests that were just rearranged in App.test.tsx - these are just notes to myself for future reviews, so that I don't go looking for them again. No need to respond to those ones!)

vdonato

Overall this LGTM!

I left some comments, but they're more or less a bunch of nits / small things (and a good number of them seem to overlap with Tim's recent review so may be outdated now). I don't think I have any concerns about the overall approach here / things look more or less like I would have guessed

frontend/src/app/components/AppContext.tsx

frontend/src/lib/hostComm/HostCommunicationManager.test.tsx

frontend/src/lib/hostComm/HostCommunicationManager.tsx

frontend/src/app/App.test.tsx

vdonato · 2023-06-01T00:26:58Z

Another thing that I think needs to be done (possibly even after a merge to develop assuming the release timeline doesn't mean that we'd immediately release this before thorough testing can be finished) would be to take some very thorough testing passes on Community Cloud, etc, potentially with the help of the QA team

Not sure how much we can fully test auth-token related things, but I have a small toy app that I used to test those changes locally that I can clean up and send your way with instructions tomorrow

tconkling

Good stuff!

frontend/src/app/App.test.tsx

* develop: Remove tensorflow and pytorch from test requirements (streamlit#6807) Fix readme links (streamlit#6800) Update PR template to be simpler and more approachable (streamlit#6679) README updated with links to docs (streamlit#6780) Refactor: withHostCommunication -> HostCommunicationManager (streamlit#6746) Add python 3.11 to classifiers list. (streamlit#6786) Release/1.23.1 (streamlit#6777) We depend on typing-extensions 4.0.1 (streamlit#6776) Release/1.23.0 (streamlit#6773) Fix typo in data_editor docstring and deprecation msg: `edited_rows` -> `edited_cells` (streamlit#6770) Fix: Remove flaky date input calendar snapshot test (streamlit#6769) Fix 3218, patch pydantic in_ipython function (streamlit#6664) Upgrade react-range to fix memory usage of sliders (streamlit#6764) Make st.write pretty-print dataclasses using st.help (streamlit#6750) Replace curly with straightquotes in docstring examples (streamlit#6757)

mayagbarnes added 9 commits May 25, 2023 09:34

First pass hostCommMgr

58b9860

Add other message types & state

aaa7a54

Remove HOC & this.props.hostCommunication references

27b1f78

Bind functions & sort tests

a55936c

Add HostCommunicationManager tests

b5d30e5

Fix App.tsx tests

ab8d025

Remove old HOC

643c844

Fix lints

c980782

Fix imports, extend manual tests

7ce276a

mayagbarnes force-pushed the feature/host-com branch from 09a12cb to 4c731bb Compare May 25, 2023 16:37

github-advanced-security bot found potential problems May 25, 2023

View reviewed changes

frontend/src/lib/hostComm/HostCommunicationManager.tsx Fixed Show fixed Hide fixed

mayagbarnes force-pushed the feature/host-com branch from 4c731bb to e111339 Compare May 25, 2023 17:35

streamlit deleted a comment from sfc-gh-mbarnes May 26, 2023

pageLinkBaseUrl to context & update hostframe for manual testing

7cdfb9c

mayagbarnes force-pushed the feature/host-com branch from a8d4e94 to 7cdfb9c Compare May 27, 2023 05:04

Update notes and comment out testing code

6d4effd

mayagbarnes force-pushed the feature/host-com branch from 6ad52ec to 6d4effd Compare May 30, 2023 05:10

mayagbarnes marked this pull request as ready for review May 30, 2023 17:49

tconkling self-requested a review May 30, 2023 22:10

tconkling suggested changes May 30, 2023

View reviewed changes

mayagbarnes added 2 commits May 30, 2023 23:09

Address HostCommMgr comments

67ae9e8

Address (most) App comments

969c401

github-advanced-security bot found potential problems May 31, 2023

View reviewed changes

mayagbarnes added 2 commits May 31, 2023 13:13

Fix jslint

c508ad5

Update HostCommMgr to use setState callbacks

d14ce8e

tconkling self-requested a review May 31, 2023 23:16

tconkling suggested changes May 31, 2023

View reviewed changes

vdonato approved these changes Jun 1, 2023

View reviewed changes

mayagbarnes added 2 commits May 31, 2023 19:03

Address comments - P2

2ec07d4

Comment out local testing code, fix js test order

1d93f62

mayagbarnes added the security-assessment-completed Security assessment has been completed for PR label Jun 1, 2023

mayagbarnes added 2 commits June 1, 2023 15:34

Merge branch 'develop' into feature/host-com

7dbfc48

Update comments

0bd7994

tconkling approved these changes Jun 2, 2023

View reviewed changes

frontend/src/app/App.test.tsx Outdated Show resolved Hide resolved

Remove TODO comment

f4e05f2

mayagbarnes merged commit 8750679 into develop Jun 2, 2023

mayagbarnes deleted the feature/host-com branch June 2, 2023 22:09

Refactor: withHostCommunication -> HostCommunicationManager #6746

Refactor: withHostCommunication -> HostCommunicationManager #6746

Uh oh!

Conversation

mayagbarnes commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Description

🧪 Testing Done

Uh oh!

Uh oh!

mayagbarnes commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayagbarnes May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

vdonato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vdonato commented Jun 1, 2023

mayagbarnes commented May 25, 2023 •

edited

Loading

mayagbarnes commented May 30, 2023 •

edited

Loading

mayagbarnes May 31, 2023 •

edited

Loading