Skip to content

Add a snooze option for port discovery #100

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

Open
marwan-at-work wants to merge 7 commits into
base: main
Choose a base branch
from
+37 −8
Open

Add a snooze option for port discovery #100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b1d651
Make port discovery optional via settings
marwan-at-work Jun 30, 2023
2e4daa2
Merge branch 'main' into marwan/snooze
tendstofortytwo Sep 27, 2023
4708ad7
Merge branch 'main' into marwan/snooze
tendstofortytwo Sep 28, 2023
b39abe4
store snooze timer to disk
tendstofortytwo Sep 28, 2023
5f7a15d
fix sign
tendstofortytwo Oct 6, 2023
0fd92c3
fix text content
tendstofortytwo Oct 6, 2023
0a4b3f9
fix readme error
tendstofortytwo Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ If the extension isn't working, we recommend following these steps to troublesho
2. Ensure that your Tailnet access controls (ACLs) are [configured to allow Tailscale Funnel](https://tailscale.com/kb/1223/tailscale-funnel/#setup) on your device.
3. Ensure that [magicDNS and HTTPS Certificates are enabled](https://tailscale.com/kb/1153/enabling-https/) on your tailnet.
4. If you are running `tailscaled` in a non-default path, you can set its path via the `tailscale.socketPath` setting in VS Code.

(Make port discovery optional via settings)
## Contribute

We appreciate your help! For information on contributing to this extension, refer to the [CONTRIBUTING](CONTRIBUTING.md) document.
Expand Down
29 changes: 27 additions & 2 deletions src/tailscale/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class Tailscale {
private notifyExit?: () => void;
private socket?: string;
private ws?: WebSocket;
private snoozing?: boolean;

constructor(vscode: vscodeModule) {
this._vscode = vscode;
Expand Down Expand Up @@ -363,13 +364,30 @@ export class Tailscale {
if (msg.type != 'newPort') {
return;
}
if (this.snoozing) {
return;
}
const shouldServe = await this._vscode.window.showInformationMessage(
msg.message,
{ modal: false },
'Serve'
'Serve',
'Snooze Notifications'
);
if (shouldServe) {
if (shouldServe === 'Serve') {
await this.runFunnel(msg.port);
} else if (shouldServe === 'Snooze Notifications') {
this.snooze();
const openSettings = await this._vscode.window.showInformationMessage(
'Snoozed for 15 minutes. You can fully turn off port discovery in the settings',
{ modal: false },
'Open Settings'
);
if (openSettings) {
this._vscode.commands.executeCommand(
'workbench.action.openSettings',
'tailscale.portDiscovery.enabled'
);
}
}
});
this._vscode.window.onDidOpenTerminal(async (e: vscode.Terminal) => {
Expand Down Expand Up @@ -401,6 +419,13 @@ export class Tailscale {
});
}

async snooze() {
this.snoozing = true;
setTimeout(() => {
Copy link
Copy Markdown

@tylersmalley tylersmalley Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is only going to snooze for 15 minutes within the instance of VS Code it was triggered in. I would like a prompt for the duration, including day, week, and month. For this, I am thinking we could use context.globalStoragePath and persist the port config (snooze info) to disk.

Thoughts @samlinville @christine-tailscale

Copy link
Copy Markdown

@christine-tailscale christine-tailscale Jul 11, 2023

Choose a reason for hiding this comment

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

When does this notification show up? Will the snooze function delay just this message or will it snooze all notifications from us?

I'm assuming that users would want to snooze because they're in the middle of something else and want to save it for later. What if instead of offering them custom snooze options, we offer them a quick yes/no decision? And then if they click no, we can use that notification to educate them where port discovery lives if they want to come back to it later?

Copy link
Copy Markdown

@tylersmalley tylersmalley Jul 11, 2023

Choose a reason for hiding this comment

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

The notification shows up as part of port discovery and as this PR stands snoozes discovery entirely:

image

Copy link
Copy Markdown
Contributor

@shayne shayne Jul 14, 2023

Choose a reason for hiding this comment

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

We should persist in the requested snooze duration (storing the time when we can unsnooze). We should not offer a calendar or complex form to fill out (we're trying to work around an annoyance, let's not annoy them more). Let's keep it simple.

15 minutes seems too short, TBH. The goal is to provide a reasonable snooze option that keeps the port disco from annoying users. If snooze is too short, we're still annoying them. The minimum risk is that the person will disable port disco notifications altogether. The max is the uninstall.

I wouldn't oppose a single "Snooze for 1hr" or "Snooze" and then prompting with 1 hour, 2 hours, or this session. (For example)

this.snoozing = false;
}, 900000); // fifteen minutes
Copy link
Copy Markdown
Contributor

@shayne shayne Jul 14, 2023

Choose a reason for hiding this comment

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

prefer const snoozeDuration = 15 * 60 * 1000 then reference the const.

}

async runFunnel(port: number) {
await this.serveAdd({
protocol: 'https',
Expand Down