Add commands for linking tsh on macOS by ravicious · Pull Request #1480 · gravitational/webapps

ravicious · 2023-01-03T11:35:57Z

This will be merged into #1445.

First I added some missing stories in order to be confidently make changes to how suggestions are displayed. Then I added the command bar commands and later added the necessary main process IPC handlers to take care of executing osascript that creates the symlinks.

It's based on how VSCode does this:

ravicious · 2023-01-03T11:37:36Z

+      }
+    });
+
+    ipcMain.handle('main-process-remove-tsh-symlink', async () => {


I just copy-pasted the other handler instead of adding some abstractions here, IMHO once we have another use case for osascript we can think about abstracting osascript prompts.

I love this thinking. Always reminds me of this.

ravicious · 2023-01-03T11:46:44Z

+    return commands.map(cmd => {
+      const acceptsNoArguments =
+        this.parserRegistry.get(cmd.displayName) instanceof
+        QuickNoArgumentsParser;


The way it worked before is that tsh ssh and tsh proxy db had their own parsers assigned to them:

webapps/packages/teleterm/src/ui/services/quickInput/quickInputService.ts

Lines 63 to 70 in 665f7e7

this.quickCommandParser.registerParserForCommand(

'tsh ssh',

new parsers.QuickTshSshParser(sshLoginSuggester, serverSuggester)

);

this.quickCommandParser.registerParserForCommand(

'tsh proxy db',

new parsers.QuickTshProxyDbParser(databaseSuggester)

);

However, tsh install and tsh uninstall don't need custom parsers as they don't accept any extra arguments. So I added QuickNoArgumentsParser that I use for those two commands.

If that parser is used for the given command, then QuickInputService is not going to append space to the input.

ravicious · 2023-01-03T11:47:38Z

 // Importing electron breaks the fixtures if that's done from within storybook.
 import { createConfigService } from 'teleterm/services/config/configService';

-const platform = 'darwin';


I had to remove the hardcoded platform in order to be able to test config service for different platforms.

This makes it easier to tell when one suggestion ends and another starts.

avatus · 2023-01-03T18:59:10Z

+
+const defaultRuntimeSettings = {
+  platform: 'darwin' as const,
+  dev: true,


I know this is a mock, but what are the consequences of having our mock be in dev mode instead of not? Could this lead to any false positives/negatives?

I know this is a mock, but what are the consequences of having our mock be in dev mode instead of not? Could this lead to any false positives/negatives?

dev is mostly used during the build & startup of the app, it shouldn't have much effect overall on tests.

In general I'm of the opinion that we shouldn't use dev for business logic, I was talking with Grzegorz about this recently. And for any other scenario, our setup code in tests should probably be closer to the dev build than the prod build (for example in terms of what's getting logged and what's not, etc.).

avatus · 2023-01-03T19:02:42Z

+      }
+    });
+
+    ipcMain.handle('main-process-remove-tsh-symlink', async () => {


I love this thinking. Always reminds me of this.

avatus · 2023-01-03T19:04:41Z

+    ipcMain.handle('main-process-remove-tsh-symlink', async () => {
+      const target = '/usr/local/bin/tsh';
+      const prompt =
+        'Teleport Connect wants to remove a symlink for tsh from /usr/local/bin.';


I wonder if this language could be changed at all. I know this is exactly what the script is doing, and I know our audience is technical enough to understand thing, but it just seems so... destructive? Idk what the word is. I'm ok with not changing it, but I wonder if there is simpler terms to describe what is happening. Maybe I'm overreacting

I'm ok with not changing it, but I wonder if there is simpler terms to describe what is happening.

We could say "Connect wants to uninstall tsh from PATH" but personally I'd prefer the app to tell me exactly what it wants to do when it asks me for a password.

Another option would be "Connect wants to modify the contents of /usr/local/bin" which doesn't use the notion of removing/uninstalling but again fails at specifying what exactly is going to happen and potentially sounds even more dangerous.

I'd probably keep this, the way I look at it is we ask the user for permission to do something so we better explain that as succinctly as possible.

zmb3 · 2023-01-03T20:10:28Z

+    // Used in the `tsh install` command on macOS to make the bundled tsh available in PATH.
+    // Returns true if tsh got successfully installed, false if the user closed the osascript
+    // prompt. Throws an error when osascript fails.
+    ipcMain.handle('main-process-symlink-tsh', async () => {


Since this is specific to macOS, should we indicate that in the name of the event?

Suggested change

ipcMain.handle('main-process-symlink-tsh', async () => {

ipcMain.handle('main-process-symlink-tsh-macos', async () => {

Since this is specific to macOS, should we indicate that in the name of the event?

I was thinking about it but in the end I didn't do it for some reason, I'll add that.

zmb3 · 2023-01-03T20:11:57Z

@@ -0,0 +1,36 @@
+import { MockAppContext } from 'teleterm/ui/fixtures/mocks';


Missing license here?

I haven't been adding license to any file since I've joined. 🙊

If that's necessary then once the merge is done we should just modify the ignored paths of make fix-license.

gzdunek · 2023-01-04T12:27:18Z

+  return (
+    <>
+      {/* Extra bottom margin to accommodate the suggestions displayed under the input. */}
+      <QuickInputDemo description="Pristine state" mb={350} />


Nit: I'd consider creating separate stories for each demo. The "scroll into viewport" mechanism in QuickInput causes the whole story to jump when you want to use an input at the bottom of the viewport.

The "scroll into viewport" mechanism in QuickInput causes the whole story to jump when you want to use an input at the bottom of the viewport.

Right, it's annoying but the reason I created the story in the first place is that I wanted to have an easy way to inspect all available suggestions.

But I suppose a better way would be to create a separate story which uses QuickInputList alone rather than setting up the whole QuickInput component. I'll make that change soon.

gzdunek · 2023-01-04T12:31:39Z

+      }
+      default: {
+        // eslint-disable-next-line @typescript-eslint/no-unused-vars
+        const exhaustiveCheck: never = command;


Don't you want to use assertUnreachable here?

Don't you want to use assertUnreachable here?

I was one the fence about it, especially when I remembered the problems we had with throwing errors when the doc type was not included in the switch.

But I think the difference between switching on docs vs commands here is that docs are loaded from a JSON file whereas commands are hardcoded.

We should be able to throw when switching on docs too but the actual issue with docs is that we don't parse the payload coming from the JSON file in any way. This allows objects of invalid type to slip through.

But that's not a problem here so yeah I think we can just always prefer assertUnreachable.

gzdunek · 2023-01-04T12:37:24Z

+it('does not return tsh install & uninstall autocomplete command on Linux', () => {
+  const appContext = new MockAppContext({ platform: 'linux' });
+  const commandLauncher = new CommandLauncher(appContext);
+  const autocompleteCommandNames = commandLauncher
+    .getAutocompleteCommands()
+    .map(c => c.name);
+
+  expect(autocompleteCommandNames).not.toContain('autocomplete.tsh-install');
+  expect(autocompleteCommandNames).not.toContain('autocomplete.tsh-uninstall');
+});
+
+it('does not return tsh install & uninstall autocomplete command on Windows', () => {
+  const appContext = new MockAppContext({ platform: 'win32' });
+  const commandLauncher = new CommandLauncher(appContext);
+  const autocompleteCommandNames = commandLauncher
+    .getAutocompleteCommands()
+    .map(c => c.name);
+
+  expect(autocompleteCommandNames).not.toContain('autocomplete.tsh-install');
+  expect(autocompleteCommandNames).not.toContain('autocomplete.tsh-uninstall');
+});


We can avoid repeating the same test code with test.each.

We can avoid repeating the same test code with test.each.

I think I'm actually going to keep it, it's just three tests and idk how to neatly refactor this with test.each without introducing additional logic since one test requires commands to be in the array and others do not.

I was thinking about using it only for windows/linux 😛

Then I'd say it's not worth it for just two short tests. 😏

gzdunek · 2023-01-04T12:56:19Z

+    description: 'Start a local proxy for a database connection',
+    run() {},
+  },
+  'autocomplete.tsh-install': {


I know that QuickInput was quickly adjusted to the new requirements before the preview release and needs some refactoring, but maybe it can be cleaned up a bit?
I'm thinking about moving autocomplete.* commands to the new variable (like autocompleteCommands). It is a bit confusing that we mix commands that actually trigger some actions with commands used only for autocomplete. Then we could add types to these autocomplete commands, so you would be able to get rid of (Array.isArray(platforms) check in commandLauncher.ts.

gzdunek · 2023-01-04T13:10:30Z

+        error => {
+          ctx.notificationsService.notifyError({
+            title: 'Could not install tsh in PATH',
+            description: `Ran into an error: ${error}`,


I didn't check it, but it seems to me that the error here is not a plain string, but an object with a message field (as we throw error from the IPC handler), so we will get a runtime error.

I didn't check it, but it seems to me that the error here is not a plain string, but an object with a message field (as we throw error from the IPC handler), so we will get a runtime error.

Errors that pass through IPC or the context bridge are still instances of Error with message, they're just stripped of any custom properties that might be attached to them. See the A word on error handling section.

The place where we discussed returning plain objects instead of errors was tshd client but we never got around to that (gravitational/webapps.e#309) 😏.

And I don't think we'd get a runtime error in any case:

const err = {message: "test"}; `err: ${err}` // > "err: [object Object]"

Errors that pass through IPC or the context bridge are still instances of Error with message, they're just stripped of any custom properties

I see, I was sure that name is passed too, as it is standard Error property.

And I don't think we'd get a runtime error in any case

Ah right, Objects are not valid as a react child happens only when rendering objects directly in JSX.

gzdunek · 2023-01-04T13:39:15Z

+        if (error instanceof Error && error.message.includes('-128')) {
+          return false;
+        }
+        this.logger.error(error);
+        throw error;


I have mixed feelings about this, it would be nice to return the errors in the same way.
Maybe we could create a new custom error, like PromptCancelledByUserError and throw (or even return) it instead of false? Then on the renderer side we would have to create our own instanceOf (as the the classes are serialized) that would compare errors by name.

IMO this would make the code more elegant, but we can live with what we have, it is quite simple and documented.

I have mixed feelings about this, it would be nice to return the errors in the same way.

The way I think about this is that closing the prompt is not really an error/exception so I wanted to reserve rejecting the promise for actual errors that might happen during the execution of the script.

As seen in your suggestion, using a custom error would be much more complicated. But even more important is that it wouldn't work because errors are not only stripped of any custom properties, they lose their name as well and become simple Error instances. So we couldn't inspect name AFAIR.

So the only way is to return plain objects, but if we don't think of closing the prompt as an error then it's fine to stay with the current solution.

gzdunek · 2023-01-04T16:16:16Z

@@ -32,10 +32,10 @@ export default function createMainProcessClient(): MainProcessClient {
      return ipcRenderer.invoke('main-process-force-focus-window');
    },
    symlinkTsh() {


I somehow forgot to send a comment earlier, but I'd also add macOs part to symlinkTsh/removeTshSymlink.

ravicious requested review from avatus and gzdunek January 3, 2023 11:35

ravicious commented Jan 3, 2023

View reviewed changes

ravicious added 6 commits January 3, 2023 12:48

Expand story for QuickInput

2bcadc7

Make command suggestions stay in place

1ba8dad

Align suggestion icons to the top rather than center

ed7cdc0

This makes it easier to tell when one suggestion ends and another starts.

Add install & uninstall cmds to command bar

5c5f8c5

Exclude new commands from OSes other than macOS

f717bf6

Implement commands for symlinking tsh

7600ae8

ravicious force-pushed the ravicious/link-tsh-macos branch from fd30885 to 7600ae8 Compare January 3, 2023 11:48

avatus approved these changes Jan 3, 2023

View reviewed changes

zmb3 approved these changes Jan 3, 2023

View reviewed changes

gzdunek reviewed Jan 4, 2023

View reviewed changes

ravicious added 4 commits January 4, 2023 14:52

Add macos suffix to symlink IPC handlers

ad11fd7

Refactor QuickInput story into two separate stories

68f0a69

Use assertUnreachable

905bd19

Refactor autocomplete commands

acf1c9f

ravicious requested a review from gzdunek January 4, 2023 15:57

gzdunek approved these changes Jan 4, 2023

View reviewed changes

Add MacOs suffix to MainProcessClient methods

b006f1f

ravicious merged commit 2daafe9 into ravicious/link-tsh Jan 4, 2023

ravicious deleted the ravicious/link-tsh-macos branch January 4, 2023 16:24

	this.quickCommandParser.registerParserForCommand(
	'tsh ssh',
	new parsers.QuickTshSshParser(sshLoginSuggester, serverSuggester)
	);
	this.quickCommandParser.registerParserForCommand(
	'tsh proxy db',
	new parsers.QuickTshProxyDbParser(databaseSuggester)
	);

	ipcMain.handle('main-process-symlink-tsh', async () => {
	ipcMain.handle('main-process-symlink-tsh-macos', async () => {

		@@ -0,0 +1,36 @@
		import { MockAppContext } from 'teleterm/ui/fixtures/mocks';

Conversation

ravicious commented Jan 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants