Add search functionality to Web UI and Connect ssh terminal by avatus · Pull Request #48776 · gravitational/teleport

avatus · 2024-11-12T00:23:41Z

This will add the xterm/search-addon addon to our terminals in the web UI ssh sessions as well as Connect. The design of the search UI was inspired by the native browser search in Chrome.

Originally, it was thought that the search addon came with a built-in UI but apparently that isn't the case, so we had to do it ourselves.

Screen.Recording.2024-11-07.at.4.50.01.PM.mov

closes: #38957

changelog: You can now search text within ssh sessions in the Web UI and Teleport Connect

ryanclark · 2024-11-12T12:39:36Z

+  function search(value: string, prev?: boolean) {
+    if (!xTerm) {
+      return;
+    }
+    const match = theme.colors.terminal.brightYellow;
+    const activeMatch = theme.colors.terminal.yellow;
+    setSearchValue(value);
+
+    xTerm.search(value, prev, {
+      decorations: {
+        matchOverviewRuler: match,
+        activeMatchColorOverviewRuler: activeMatch,
+        matchBackground: match,
+        activeMatchBackground: activeMatch,
+      },
+    });
+  }
+
+  const onChange = (e: React.ChangeEvent<HTMLInputElement>) => {
+    search(e.target.value);
+  };
+
+  const onKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => {
+    if (e.key === 'Enter') {
+      search(e.currentTarget.value);
+    }
+  };
+
+  function searchNext() {
+    search(searchValue);
+  }
+
+  function searchPrevious() {
+    search(searchValue, true /* search for previous result */);
+  }
+
+  const onEscape = useCallback(() => {
+    onClose();
+    xTerm.clearSearch();
+    xTerm.term.focus();
+  }, [xTerm, onClose]);
+
+  const onSearchResultsChange = useCallback(
+    (resultIndex: number, resultCount: number) => {
+      setSearchResults({ resultIndex, resultCount });
+    },
+    []
+  );


This has a mixture of const x = () => {}, function y() { } and const z = useCallback(() => {}, []) - we should be consistent. My personal preference is useCallback.

fair enough. i prefer function y() but i use const when doing useCallback and then a few other cheeky bare const got thrown in there. i'll switch to const and useCallback when needed. thanks

avatus · 2024-11-12T23:09:03Z

rebased due to conflict. PR feedback handled here and on a930d3e

github-actions · 2024-11-12T23:19:10Z

🤖 Vercel preview here: https://docs-6wic0fycq-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-11-12T23:23:21Z

🤖 Vercel preview here: https://docs-hyszxa56h-goteleport.vercel.app/docs/ver/preview

gzdunek · 2024-11-13T10:09:24Z

+  const isSearchKeyboardEvent = useCallback(
+    (e: KeyboardEvent) => {
+      return (
+        e.type === 'keydown' &&


We don't have this check in Web. Why is it needed?

I just saw other spots in Connect focusing specifically on keydown, didn't think too much on it. I can remove as the method being called on true is idempotent and shouldn't matter on keyup/keydown or duplicates

As discussed in DMs, let's move this check to the TerminalSearch level.

github-actions · 2024-11-13T19:27:30Z

🤖 Vercel preview here: https://docs-hh226fzu8-goteleport.vercel.app/docs/ver/preview

gzdunek · 2024-11-15T10:29:37Z

+        return false;
+      }
+      // this escape is handled if pressing escape with the term focused
+      if (e.key === 'Escape') {


I'm not sure if we need the "global" Escape handler, some terminals only close the search when you have the input focused.
But if you want to keep it, we should run it only if the search is shown. Otherwise, we will intercept all Escape key events, even when we don't need them.

I see that you added a check in onKeyDown. However, this place doesn't need to care about it at all, onKeyDown is called only when the input is open.

But we still need to address the issue in registerCustomKeyEventHandler. We don't want to intercept Escape events when the search is closed.
I actually think we should remove closing the search from there. If you take a look at the file transfer, Escape closes it only when it is focused. I think it makes sense to have the same behavior for the search.

Ah i see. Ok, I've removed it from register and now only check in onKeyDown for the search input. also, I've remove dthe show check on that because onKeyDown can only be called when the input is showing anyway. Let me know if it's good now 👍

gzdunek · 2024-11-15T10:50:58Z

+  const isSearchKeyboardEvent = useCallback(
+    (e: KeyboardEvent) => {
+      return (
+        e.type === 'keydown' &&


As discussed in DMs, let's move this check to the TerminalSearch level.

github-actions · 2024-11-15T17:00:36Z

🤖 Vercel preview here: https://docs-4jvhqgopl-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-11-16T01:00:23Z

🤖 Vercel preview here: https://docs-pl6lbgnzq-goteleport.vercel.app/docs/ver/preview

gzdunek

Left some minor comments, we are almost there!

gzdunek · 2024-11-18T14:48:43Z

+        return false;
+      }
+      // this escape is handled if pressing escape with the term focused
+      if (e.key === 'Escape') {


I see that you added a check in onKeyDown. However, this place doesn't need to care about it at all, onKeyDown is called only when the input is open.

But we still need to address the issue in registerCustomKeyEventHandler. We don't want to intercept Escape events when the search is closed.
I actually think we should remove closing the search from there. If you take a look at the file transfer, Escape closes it only when it is focused. I think it makes sense to have the same behavior for the search.

github-actions · 2024-11-18T18:18:41Z

🤖 Vercel preview here: https://docs-6xbf6tt6p-goteleport.vercel.app/docs/ver/preview

gzdunek

Left some lasts comments.

🚢

gzdunek · 2024-11-19T14:03:26Z

+ */
+
+import { render, act, screen } from 'design/utils/testing';
+import 'jest-canvas-mock';


I think 'jest-canvas-mock' and import '@testing-library/jest-dom' do nothing.

they were used before but i dont think anything is needed with them now.

gzdunek · 2024-11-19T14:04:22Z

+  })),
+}));
+
+const createMockXTerm = () => {


Let's remove the references to xterm in tests/stories as well, I'd just name it terminalMock.

gzdunek · 2024-11-19T14:06:22Z

+  }, [
+    onEscape,
+    onOpen,
+    searchInputRef,
+    terminalSearcher,
+    show,
+    isSearchKeyboardEvent,
+  ]);


There are some unnecessary deps:

Suggested change

}, [

onEscape,

onOpen,

searchInputRef,

terminalSearcher,

show,

isSearchKeyboardEvent,

]);

}, [isSearchKeyboardEvent, onOpen, terminalSearcher]);

gzdunek · 2024-11-19T14:07:12Z

  FileTransferRequests,
  FileTransferContextProvider,
 } from 'shared/components/FileTransfer';
+import { TerminalSearch } from 'shared/components/TerminalSearch/TerminalSearch';


Suggested change

import { TerminalSearch } from 'shared/components/TerminalSearch/TerminalSearch';

import { TerminalSearch } from 'shared/components/TerminalSearch';

gzdunek · 2024-11-19T14:09:53Z

  FileTransfer,
  FileTransferContextProvider,
 } from 'shared/components/FileTransfer';
+import { TerminalSearch } from 'shared/components/TerminalSearch/TerminalSearch';


Suggested change

import { TerminalSearch } from 'shared/components/TerminalSearch/TerminalSearch';

import { TerminalSearch } from 'shared/components/TerminalSearch';

gzdunek · 2024-11-19T14:43:18Z

        openProfiles: 'Ctrl+Shift+I',
        terminalCopy: 'Ctrl+Shift+C',
        terminalPaste: 'Ctrl+Shift+V',
+        terminalSearch: 'Ctrl+F',


I just checked the terminal apps on Windows and Linux and they use Ctrl+Shift+F for search. I believe it's because Ctrl+F is used to move the cursor one character forward in bash. Please switch these shortcuts from Ctrl+F in Connect to Ctrl+Shift+F (and remember about updating the docs).

FYI we will have the same problem in Web UI where we overwrite Ctrl+F. We have no choice on Linux and Windows where this is a combination that opens the native search anyway. On macOS in theory we could only intercept Meta+F and leave Ctrl+F for the terminal, but I think we don't have a reliable way to test if the user is on Mac. So I guess we have to leave it as is.

avatus · 2024-11-20T15:27:55Z

@ryanclark im about to finish gz's last comments. anything left from you?

ryanclark

all good from me

github-actions · 2024-11-20T15:59:42Z

🤖 Vercel preview here: https://docs-g9ear6eqx-goteleport.vercel.app/docs

public-teleport-github-review-bot · 2024-11-20T16:12:24Z

@avatus See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Create PR

Backport #48776 to branch/v16 changelog: You can now search text within ssh sessions in the Web UI and Teleport Connect

…9270) Backport #48776 to branch/v16 changelog: You can now search text within ssh sessions in the Web UI and Teleport Connect

avatus added backport-required backport/branch/v17 labels Nov 12, 2024

avatus requested review from gzdunek and ryanclark November 12, 2024 00:23

github-actions Bot added size/md ui labels Nov 12, 2024

github-actions Bot requested review from kimlisa and rudream November 12, 2024 00:24

avatus force-pushed the avatus/search branch from 8bb4881 to 93d9d74 Compare November 12, 2024 01:41

gzdunek reviewed Nov 12, 2024

View reviewed changes

ryanclark reviewed Nov 12, 2024

View reviewed changes

avatus force-pushed the avatus/search branch 2 times, most recently from 86cd4df to a930d3e Compare November 12, 2024 23:08

avatus temporarily deployed to vercel November 12, 2024 23:08 — with GitHub Actions Inactive

avatus temporarily deployed to vercel November 12, 2024 23:12 — with GitHub Actions Inactive

avatus commented Nov 13, 2024

View reviewed changes

Comment thread web/packages/shared/components/TerminalSearch/TerminalSearch.tsx Outdated

gzdunek reviewed Nov 13, 2024

View reviewed changes

avatus temporarily deployed to vercel November 13, 2024 19:16 — with GitHub Actions Inactive

gzdunek reviewed Nov 15, 2024

View reviewed changes

avatus temporarily deployed to vercel November 15, 2024 16:50 — with GitHub Actions Inactive

avatus requested a review from gzdunek November 15, 2024 16:50

avatus temporarily deployed to vercel November 16, 2024 00:49 — with GitHub Actions Inactive

gzdunek reviewed Nov 18, 2024

View reviewed changes

avatus temporarily deployed to vercel November 18, 2024 18:08 — with GitHub Actions Inactive

gzdunek approved these changes Nov 19, 2024

View reviewed changes

avatus requested a review from ryanclark November 20, 2024 15:28

ryanclark approved these changes Nov 20, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed request for kimlisa and rudream November 20, 2024 15:29

Add xterm/search-addon package and search UI

71a9914

avatus force-pushed the avatus/search branch from 39f2cde to 71a9914 Compare November 20, 2024 15:49

avatus temporarily deployed to vercel November 20, 2024 15:49 — with GitHub Actions Inactive

avatus enabled auto-merge November 20, 2024 15:49

avatus added the backport/branch/v16 label Nov 20, 2024

avatus added this pull request to the merge queue Nov 20, 2024

Merged via the queue into master with commit ad5963a Nov 20, 2024

avatus deleted the avatus/search branch November 20, 2024 16:10

avatus mentioned this pull request Nov 20, 2024

[v17] Add search functionality to Web UI and Connect ssh terminal #49269

Merged

avatus added a commit that referenced this pull request Nov 20, 2024

[v16] Add search functionality to Web UI and Connect ssh terminal

558b62b

Backport #48776 to branch/v16 changelog: You can now search text within ssh sessions in the Web UI and Teleport Connect

avatus mentioned this pull request Nov 20, 2024

[v16] Add search functionality to Web UI and Connect ssh terminal #49270

Merged

avatus added a commit that referenced this pull request Nov 20, 2024

[v16] Add search functionality to Web UI and Connect ssh terminal

795e0d3

Backport #48776 to branch/v16 changelog: You can now search text within ssh sessions in the Web UI and Teleport Connect

	import { TerminalSearch } from 'shared/components/TerminalSearch/TerminalSearch';
	import { TerminalSearch } from 'shared/components/TerminalSearch';

Conversation

avatus commented Nov 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus commented Nov 12, 2024

Uh oh!

github-actions Bot commented Nov 12, 2024

Uh oh!

github-actions Bot commented Nov 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

github-actions Bot commented Nov 13, 2024

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 15, 2024

Uh oh!

github-actions Bot commented Nov 16, 2024

Uh oh!

gzdunek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 18, 2024

Uh oh!

gzdunek left a comment

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