Skip to content
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(target): disable target creation if connectUrl is left empty #566

Merged
merged 11 commits into from
Oct 26, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Oct 26, 2022

Fixes #565

Fixes

  • Disable target creation if connectUrl is left empty in both cases (i.e. clicking Create button and pressing Enter).
  • Target labels/annotations are changed to generic object since JSON is not mapped into this Map type and fields are added as general support for any objects. See here.

Chores

  • Remove subscription to JVMDiscovery notification since context.target.targets will be called to return new array of targets.
  • Refactor to use common local storage utilities.

@tthvo tthvo added chore Refactor, rename, cleanup, etc. fix labels Oct 26, 2022
@andrewazores
Copy link
Member

Testing this out manually, it seems like something is causing a LOT of repeated API requests to the backend. I guess some component is getting re-rendered continuously and causing a request or series of requests each time. Just visiting the Recordings or Events views and selecting a target is enough to trigger it.

@tthvo
Copy link
Member Author

tthvo commented Oct 26, 2022

Testing this out manually, it seems like something is causing a LOT of repeated API requests to the backend. I guess some component is getting re-rendered continuously and causing a request or series of requests each time. Just visiting the Recordings or Events views and selecting a target is enough to trigger it.

Ah yess I saw it now. Sorry didnt see it yesterday. I will have a look to see which commit is causing this.

@tthvo tthvo marked this pull request as draft October 26, 2022 17:28
@tthvo
Copy link
Member Author

tthvo commented Oct 26, 2022

Seemed like I ran into the classic indefinite effects due to ever-changing deps for effect hooks. Fixed it now :D but I am checking an odd warning when select another target from no target-selected state.

Cannot update a component (`TargetView`) while rendering a different component (`TargetSelect`). To locate the bad setState() call inside `TargetSelect`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

@tthvo
Copy link
Member Author

tthvo commented Oct 26, 2022

It should be ready for reviews again now....

Seemed like I ran into the classic indefinite effects due to ever-changing deps for effect hooks. Fixed it now :D but I am checking an odd warning when select another target from no target-selected state.

Cannot update a component (`TargetView`) while rendering a different component (`TargetSelect`). To locate the bad setState() call inside `TargetSelect`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

The issue for this is that i was trying to avoid depending on the current target state of TargetSelect as callback will change if state changes, so I used setState which returns the currently selected target. However, we are calling context.target.setTarget() from inside here, which triggers other components to render and React is pretty much against this.

So, I reverted back to use state and added a comment to warn about this. Perhaps after #429, we can resolve this....

@tthvo tthvo marked this pull request as ready for review October 26, 2022 19:29
@andrewazores andrewazores merged commit 057e27e into cryostatio:main Oct 26, 2022
mergify bot pushed a commit that referenced this pull request Oct 26, 2022
* fix(target): should not create target if targetUrl is empty

* chore(target): clean up

* chore(target): do not use Map for labels/annotations

* tests(target): adjust unit tests

* chore(target): apply prettier

* fix(target): fix undeclared vars error

* tests(target): add missing mocks

* fix(target): fix indefinite effects

* chore(target): apply prettier

* chore(target): clean up hook deps

* fix(target): remove setState

(cherry picked from commit 057e27e)
andrewazores pushed a commit that referenced this pull request Oct 26, 2022
… (#567)

* fix(target): should not create target if targetUrl is empty

* chore(target): clean up

* chore(target): do not use Map for labels/annotations

* tests(target): adjust unit tests

* chore(target): apply prettier

* fix(target): fix undeclared vars error

* tests(target): add missing mocks

* fix(target): fix indefinite effects

* chore(target): apply prettier

* chore(target): clean up hook deps

* fix(target): remove setState

(cherry picked from commit 057e27e)

Co-authored-by: Thuan Vo <[email protected]>
@tthvo tthvo deleted the target-select branch October 27, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport chore Refactor, rename, cleanup, etc. fix
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Target creation allows empty connectUrl when users press Enter key
2 participants