Skip to content

Patch: Allow option to disable telemetry #362

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.

Sign up for GitHub

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

Jump to bottom
Open
nisrulz wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Patch: Allow option to disable telemetry #362

nisrulz wants to merge 5 commits into from

Conversation

@nisrulz
Copy link

@nisrulz nisrulz commented Aug 11, 2025 •
edited
Loading

Note

I am aware the telemetry in place is for a reason as mentioned in the readme
I just think in all fairness, the user should have a choice to disable it if they care about it so much. This is why the sane default it still true to respect the original developer's choice.
Also the original developer did express that they are open to this kind of PR.
Screenshot 2025-08-11 at 19 04 26

Testing

I verified it by running

npm run start

..and checking the logs.

Some screenshots

Screenshot 2025-08-11 at 18 50 33 Screenshot 2025-08-11 at 18 50 45

Logs:

Screenshot 2025-08-11 at 18 51 04 Screenshot 2025-08-11 at 18 51 12

@volcbs volcbs mentioned this pull request Oct 16, 2025
Merged
@actuallymentor
Copy link
Owner

actuallymentor commented Oct 21, 2025

Hey @nisrulz!

Thanks for this PR. I'm entirely open to merging it. Your code however also blocks the built in update. The setting check should only block unidentified analytics. The calls to Github and Icanhazip are used to see if the laptop is online and can thus check for updates safely.

nisrulz
nisrulz commented Oct 22, 2025
View reviewed changes
app/modules/battery.js Outdated
Comment on lines 128 to 140
// Check for network
const online = await Promise.race( [
exec_async( `${ path_fix } curl -I https://icanhazip.com &> /dev/null` ).then( () => true ).catch( () => false ),
exec_async( `${ path_fix } curl -I https://github.com &> /dev/null` ).then( () => true ).catch( () => false )
] )
log( `Internet online: ${ online }` )
// Check for network only if telemetry is enabled
let online = false;
if (get_telemetry_setting()) {
online = await Promise.race([
exec_async(`${path_fix} curl -I https://icanhazip.com &> /dev/null`).then(() => true).catch(() => false),
exec_async(`${path_fix} curl -I https://github.com &> /dev/null`).then(() => true).catch(() => false)
]);
log(`Internet online: ${online}`);
} else {
log('Internet check skipped because telemetry is not enabled');
}
Copy link
Author

@nisrulz nisrulz Oct 22, 2025

Choose a reason for hiding this comment

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

@actuallymentor are you pointing to this code? If I revert this part, then the PR would align with what you are expecting, correct?

Copy link
Author

@nisrulz nisrulz Oct 22, 2025

Choose a reason for hiding this comment

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

My reasoning to put this also behind the flag, was to make sure the app was completely offline.

Would you be okay, if I added another option "Offline Mode", that can give the user option to go completely offline. The sane default would be false, but if the user wants to choose to go into offline mode, they are aware that no update would be checked for.

Copy link
Author

@nisrulz nisrulz Oct 22, 2025

Choose a reason for hiding this comment

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

I have updated my PR to revert the change.

If you agree then, I can open a new one for "Offline mode".

@actuallymentor
Copy link
Owner

actuallymentor commented Oct 22, 2025

Looks good! Is there a reason you made the click function async? You're not doing any async ops right?

@nisrulz
Copy link
Author

nisrulz commented Oct 22, 2025

Looks good! Is there a reason you made the click function async? You're not doing any async ops right?

I actually took inference from existing menu item before it:

{
                        label: `Allow force-discharging`,
                        type: 'checkbox',
                        checked: allow_discharge,
                        click: async () => {
                            const success = await update_force_discharge_setting()
                            if( limiter_on && success ) await restart_limiter()
                        }
                    }

but you are right, making it async is not required.

I will remove and retest everything again.

@nisrulz
Copy link
Author

nisrulz commented Oct 23, 2025

@actuallymentor I have updated the PR.

@nisrulz
Copy link
Author

nisrulz commented Nov 13, 2025 •
edited
Loading

@actuallymentor do you want me to make more changes or is the PR in good shape now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nisrulz @actuallymentor