Conversation
avatus
left a comment
There was a problem hiding this comment.
Works great for me locally, but I haven't finished looking at all the code yet. Ill return to this on monday
ravicious
left a comment
There was a problem hiding this comment.
I'll continue the review tomorrow. I wanted to look around different documents and then got stuck at some weird errors related to access requests that I managed to solve. (I think some watchers got broken when my laptop woke up from sleep and I had to restart the cluster)
There was a problem hiding this comment.
Thoughts on the optimized version of the hook?
Because hooks cannot be called conditionally, we can't do early returns. But in return (hehe) the component is indeed re-rendered only when necessary, instead on every change to workspaces, clusters and gateways.
Details
import { ComponentType, useCallback } from 'react';
import { IconProps } from 'design/Icon/Icon';
import {
getResourceUri,
getStaticNameAndIcon,
} from 'teleterm/ui/services/workspacesService';
import { routing } from 'teleterm/ui/uri';
import { useStoreSelector } from '../hooks/useStoreSelector';
interface Breadcrumb {
name: string;
Icon?: ComponentType<IconProps>;
}
export function useActiveDocumentClusterBreadcrumbs(): Breadcrumb[] {
const activeDocument = useStoreSelector(
'workspacesService',
useCallback(state => {
const workspace = state.workspaces[state.rootClusterUri];
return workspace?.documents.find(d => d.uri === workspace?.location);
}, [])
);
const resourceUri = activeDocument && getResourceUri(activeDocument);
const staticNameAndIcon =
activeDocument && getStaticNameAndIcon(activeDocument);
const clusterUri = resourceUri && routing.ensureClusterUri(resourceUri);
const rootClusterUri =
resourceUri && routing.ensureRootClusterUri(resourceUri);
const cluster = useStoreSelector(
'clustersService',
useCallback(state => state.clusters.get(clusterUri), [clusterUri])
);
const rootCluster = useStoreSelector(
'clustersService',
useCallback(state => state.clusters.get(rootClusterUri), [rootClusterUri])
);
if (!cluster || !rootCluster || !staticNameAndIcon) {
return;
}
return [
{ name: rootCluster.name },
clusterUri !== rootClusterUri && { name: cluster.name },
{
name: staticNameAndIcon.name,
Icon: staticNameAndIcon.Icon,
},
].filter(Boolean);
}There was a problem hiding this comment.
Much better, thank you!
There was a problem hiding this comment.
I mean, I'm not sure if I'm a fan of this style of writing hooks/components. But it's hard to argue against the performance implications of it.
Maybe that's what people arguing in favor of signals mean, idk. As in, maybe in another paradigm we wouldn't have to worry not being able to return early.
ravicious
left a comment
There was a problem hiding this comment.
Looks pretty slick! And the icons don't make the UI look busy, now that I used it myself. I think the demo looked busy mostly because it had a small window and a lot of tabs to show how you solved the problem of showing/hiding the close button. But in a "real" app it looks pretty cool.
…elpers in docs helpers
# Conflicts: # web/packages/teleterm/src/ui/services/connectionTracker/connectionTrackerService.ts # web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsService.ts # web/packages/teleterm/src/ui/services/workspacesService/documentsService/documentsUtils.ts
|
Ah, I forgot to re-request the review! |
|
I wouldn't have time to re-review it earlier anyway. 😂 😭 |
| const newItem = createGatewayConnection(doc); | ||
| draft.connections.push(newItem); | ||
| } else { | ||
| // In case the document changes, update the gateway title. |
There was a problem hiding this comment.
I don't think this is specific enough. IIRC this was to specifically address a case where over time we've decided to change the title for db gateways and we wanted this change to be reflected in already created connections, right?
There was a problem hiding this comment.
Yeah, I mentioned it now it the comment.
There was a problem hiding this comment.
lol ok, when I was suggesting this, I completely forgot that TabHost renders tabs and documents. I'm glad adding this wasn't much of a problem and it's indeed pretty cool to see those docs rendered in the browser.
* Show active document in the status bar * Update reading breadcrumbs value in tests * Show db name first and then username * Add helpers for making documents * Show document icon in tabs and adjust styling * Update gateway title from the document title * Fix document helpers * Put breadcrumbs in title * Simplify breadcrumb key * Optimize `useActiveDocumentClusterBreadcrumbs` * Deprecate `isDocumentTshNodeWithLoginHost` and `isDocumentTshNodeWithServerId` * Remove `aria-label="breadcrumbs"` * Replace id with class * Reuse `makeAppGateway`, `makeDatabaseGateway` and `makeKubeGateway` helpers in docs helpers * Add a comment about updating gateway title * Re-update the format of db gateways title * Render entire TabHost instead of only tabs * Make comment more specific (cherry picked from commit d1862f3)
* Show active document in the status bar * Update reading breadcrumbs value in tests * Show db name first and then username * Add helpers for making documents * Show document icon in tabs and adjust styling * Update gateway title from the document title * Fix document helpers * Put breadcrumbs in title * Simplify breadcrumb key * Optimize `useActiveDocumentClusterBreadcrumbs` * Deprecate `isDocumentTshNodeWithLoginHost` and `isDocumentTshNodeWithServerId` * Remove `aria-label="breadcrumbs"` * Replace id with class * Reuse `makeAppGateway`, `makeDatabaseGateway` and `makeKubeGateway` helpers in docs helpers * Add a comment about updating gateway title * Re-update the format of db gateways title * Render entire TabHost instead of only tabs * Make comment more specific (cherry picked from commit d1862f3)
* Show active document in the status bar * Update reading breadcrumbs value in tests * Show db name first and then username * Add helpers for making documents * Show document icon in tabs and adjust styling * Update gateway title from the document title * Fix document helpers * Put breadcrumbs in title * Simplify breadcrumb key * Optimize `useActiveDocumentClusterBreadcrumbs` * Deprecate `isDocumentTshNodeWithLoginHost` and `isDocumentTshNodeWithServerId` * Remove `aria-label="breadcrumbs"` * Replace id with class * Reuse `makeAppGateway`, `makeDatabaseGateway` and `makeKubeGateway` helpers in docs helpers * Add a comment about updating gateway title * Re-update the format of db gateways title * Render entire TabHost instead of only tabs * Make comment more specific
Currently, if you open multiple tabs, their titles may be truncated, making it difficult to see what is the resource name.
To improve this, we can show the document (resource) name in the status bar:
Other changes:
This extra icon unfortunately takes up valuable space in the tabs, so to avoid trimming the titles even further, I hid the close buttons. It is now only visible when the tab is active or when you hover over it. Thanks to this, we show ~the same amount of text as before.
Problems
The document name in the status bar shouldn't always matches the title in the tabs. For example, we don't want to show cwd or shell name in the status bar for the terminal tabs. Because of that, we shouldn't blindly use the title of the documents, but instead build the name separately.
Unfortunately, there were some problems with this approach:
doc.terminal_tsh_nodeonly the document title contains the server name (in the fields we have ID). It's because we update the title asynchronously when we fetch the server name from the backend.doc.clusterwe shouldn't show cluster name in the status bar, since it would look like (teleport-17-ent.asteroid.earth > teleport-17-ent.asteroid.earth).These problems would be solved by getting rid of storing the document title on disk, and instead generating it dynamically using a function similar to
getStaticNameAndIcon. That's quite a lot work to refactor this, so I decided to reuse the document title when possible, and when not, generate it from the document fields.changelog: Teleport Connect now shows a resource name in the status bar