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

streamer mode fix #512

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

grahambrown11
Copy link
Contributor

Toggle the streamer mode in the add-on config to protect your secrets:
image

@grahambrown11 grahambrown11 mentioned this pull request Jul 18, 2023
11 tasks
@grahambrown11
Copy link
Contributor Author

Resolves one part of esphome/issues#4709

</style>
`;
const isSecrets = this._isSecrets();
if (isSecrets && this.streamerMode && !this._showSecrets) {
Copy link
Member

Choose a reason for hiding this comment

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

This feels weird to put it in this component. Instead, this dialog should be it’s own code that is called from the “Secrets” button. When user confirms, it will continue to open the editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could be an approach, but this will work for when I've implemented routing the secrets page will have it;s own url /secrets the button will be a link to the page not a change in state...

Copy link
Member

Choose a reason for hiding this comment

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

We can always attach an event listener to the link to avoid it linking in streamer mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why you're not ok with this being in the editor.
There was already logic for the editor to handle secrets differently (no install & ace websocket not started)
so I don't see why it cannot be given 1 extra parameter (streamer mode) in order to show the warning 1st

Copy link
Member

Choose a reason for hiding this comment

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

A component should do 1 thing. This makes it very complicated with doing completely different things. Also, it's a prompt to a user so we should follow the convention of asking that as a dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK created a dialog in streamer-warning.ts
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob are you happy with this change?

src/util/console-color.ts Outdated Show resolved Hide resolved
src/util/console-color.ts Outdated Show resolved Hide resolved
@@ -3,6 +3,10 @@ import { StreamError, streamLogs } from "../api";
import { ColoredConsole, coloredConsoleStyles } from "../util/console-color";
import { fireEvent } from "../util/fire-event";

export class ESPHomeBlurSecrets {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this inside remote-process ? This can be in it's own file and doesn't have to be a class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I din't think it needed its own file as it is so small - and it needs to be a class as it is used as a static/global variable, I find this a better approach than using the window object to set globals...

@@ -101,9 +112,24 @@ class ESPHomeEditor extends LitElement {
></mwc-button>`}
</div>
<main></main>
${isSecrets && this.streamerMode && !this._showSecrets
? html`<streamer-warning
Copy link
Member

Choose a reason for hiding this comment

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

It should be it's own dialog like how we open the wizard. The editor shouldn't know it's opening a secret. Just the header should, if streamer mode enabeld, prompt to confirm the user wants that before opening the editor. The editor should not be aware of streamer mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The editor is already aware of secrets - thats not something new...
And as mentioned before, I think this is better here for when the secrets have its own URL (the other part I am working on), the header wont be able to show a popup if the user went directly to /secrets...

Copy link
Member

Choose a reason for hiding this comment

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

The only way a user can go to secrets is by pressing the button in the top toolbar. If they go any other way, streamer mode does not have to guard for that. The dialog needs to be it's own file.

@7wells
Copy link

7wells commented Sep 30, 2024

Sorry to open this again, but streamer_mode: true does not seem to do anything for me, i.e. I can still see e.g. the SSID in the log. Have I maybe misunderstood something? What can I add here to help? (if needed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants