-
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
feat(jmx): re-implement enhanced transient JMX credentials #524
feat(jmx): re-implement enhanced transient JMX credentials #524
Conversation
dd41789
to
77a0cf6
Compare
77a0cf6
to
ad1e169
Compare
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.
Everything else looks good, the ability to choose whether credentials are stored in backend or per session works well, there is something somewhat unrelated but there seems to be a memory-leak with the JMXAuthForm (but is a component in AuthModal) even though it wasn't touched in this PR.
This gets printed to the console
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
JmxAuthForm@webpack-internal:///./src/app/AppLayout/JmxAuthForm.tsx:53:54
// etc... etc...
I think it's just that this subscription on line 61 isn't closed.
const handleSave = React.useCallback(() => {
props.onSave(username, password).then(() => {
clear();
context.target.setAuthRetry(); // <--------------
});
}, [context, context.target, clear, props.onSave, username, password]);
Hmm, I don't think there's a subscription there... that method just returns @tthvo has seen and investigated these "state update on an unmounted component" messages lately. Any insights here? |
Oh, whoops my bad, then it's something else internal then I guess, hm... |
Ohh yesss, here:
There is a call to To fix, just remove it as it is not needed as the form is mounted fresh anyway. |
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! Just a couple comments. Also, it seems there are no current unit tests for this Setting
view. I think we should add them :D
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 great to me!
Fixes #523 . See there for a description of why this change is useful/necessary and the historical context.
TODO later, maybe: implement a client-side keyring. ie store the JMX credentials in local storage with a user-provided encryption key.