-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix(targets): hold targets state in new service #247
Conversation
Query /api/v1/targets at web-client startup to find initial list of known targets state. This list is updated incrementally via notification channel updates asynchronously, which the TargetSelect component receives via RxJS Observable emissions to update the graphical state accordingly. This removes the need for a GET /api/v1/targets on each in-app navigation (when the TargetSelect component is re-rendered), increasing page responsiveness and fluidity.
How do I start the web client on your branch? The web client's API requests fail, eg
|
You'll need to disable SSL as well. CRYOSTAT_CORS_ORIGIN=http://localhost:9000 CRYOSTAT_DISABLE_SSL=true CRYOSTAT_DISABLE_JMX_AUTH=true sh smoketest.sh to start the backend, and yarn yarn:frzinstall && yarn start:dev to start the web-client frontend. |
Checking my understanding here - |
Pretty much.
Meanwhile, the |
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.
Works as described. Very nice!
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 good!
Fixes #246
Query
/api/v1/targets
at web-client startup to find initial list of knowntargets state. This list is updated incrementally via notification channel
updates asynchronously, which the
TargetSelect
component receives via RxJSObservable emissions to update the graphical state accordingly.
This removes the need for a
GET /api/v1/targets
on each in-app navigation(when the TargetSelect component is re-rendered), increasing page
responsiveness and fluidity. Update handling and state logic is also decoupled
from presentational React component and moved into the TargetsService, making
the component smaller and easier to understand/more maintainable.
To test, open your browser's dev tools panel and go to the Network tab (or
simply watch Cryostat backend logs). Click between Recordings and Events or
any other in-app navigation between pages that contain the TargetSelect
component at the top. On the current upstream main branch, observe that the
browser/logs record a
GET /api/v1/targets
(or even two, actually) on eachin-app navigation. On this PR branch, perform the same test and observe that
only one
GET
is performed at the application startup. The + and trashcanicons can be used to test the custom target creation/deletion, which should
now update the UI without performing any
GET
.podman kill/podman run
(if running the backend using
smoketest.sh
) can be used to trigger asyncdiscovery
FOUND/LOST
events as well, which should also cause the UI toupdate without a
GET
query. Clicking the refresh circular-arrow icon onthe TargetSelect should prompt a single
GET /api/v1/targets
query. Enablingview auto-refresh (gear icon on top nav bar) should cause singular
GET
s tooccur at the specified refresh interval.