Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions packages/teleport/src/DesktopSession/DesktopSession.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ const props: State = {
authenticate: () => {},
setState: () => {},
},
directorySharingBrowserErr: false,
setDirectorySharingBrowserErr: () => {},
isUsingChrome: true,
};

export const Processing = () => (
Expand Down
49 changes: 31 additions & 18 deletions packages/teleport/src/DesktopSession/DesktopSession.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ declare global {

export function DesktopSession(props: State) {
const {
directorySharingBrowserErr,
setDirectorySharingBrowserErr,
clipboardState,
fetchAttempt,
tdpConnection,
Expand All @@ -62,21 +64,20 @@ export function DesktopSession(props: State) {
// onDialogClose is called when a user
// dismisses a non-fatal error dialog.
const onDialogClose = () => {
// This setTdpConnection call will cause the useEffect below
// to calculate the errorDialog state.
// The following state-setting calls will
// cause the useEffect below to calculate the
// errorDialog state.

setTdpConnection(prevState => {
// onDialogClose should only be called when
// the user dismisses a non-fatal error dialog,
// and prevState.status === '' means non-fatal
// error dialog, so the below if statement should
// always be true.
if (prevState.status === '') {
// If prevState.status was a non-fatal error,
// we assume that the TDP connection remains open.
return { status: 'success' };
}
return prevState;
});

setDirectorySharingBrowserErr(false);
};

const computeErrorDialog = () => {
Expand All @@ -100,9 +101,16 @@ export function DesktopSession(props: State) {
errorText = clipboardState.errorText || 'clipboard sharing failed';
} else if (unknownConnectionError) {
errorText = 'Session disconnected for an unknown reason.';
} else if (directorySharingBrowserErr) {
errorText =
'Your user role supports directory sharing over desktop access, \
however this feature is only available by default on some chromium \
Comment thread
ibeckermayer marked this conversation as resolved.
Outdated
based browsers like Google Chrome or Microsoft Edge. Brave users can \
use the feature by navigating to brave://flags/#file-system-access-api \
and selecting "Enable". Please switch to a supported browser.';
}
const open = errorText !== '';
const fatal = tdpConnection.status !== '';
const fatal = !(tdpConnection.status === '' || directorySharingBrowserErr);

return { open, text: errorText, fatal };
};
Expand Down Expand Up @@ -197,6 +205,7 @@ function Session(props: PropsWithChildren<State>) {
canShareDirectory,
isSharingDirectory,
setIsSharingDirectory,
setDirectorySharingBrowserErr,
onPngFrame,
onClipboardData,
onTdpError,
Expand Down Expand Up @@ -229,16 +238,20 @@ function Session(props: PropsWithChildren<State>) {
clipboardSuccess;

const onShareDirectory = () => {
window
.showDirectoryPicker()
.then(sharedDirHandle => {
setIsSharingDirectory(true);
tdpClient.addSharedDirectory(sharedDirHandle);
tdpClient.sendSharedDirectoryAnnounce();
})
.catch(() => {
setIsSharingDirectory(false);
});
try {
window
.showDirectoryPicker()
.then(sharedDirHandle => {
setIsSharingDirectory(true);
tdpClient.addSharedDirectory(sharedDirHandle);
tdpClient.sendSharedDirectoryAnnounce();
})
.catch(() => {
setIsSharingDirectory(false);
});
} catch (e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not used to seeing a promise's .catch used in conjunction with a standard catch block. Are both of them going to be called in the event of an error?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Are both of them going to be called in the event of an error?

No, what this means is if the Promise returned by window.showDirectoryPicker() is rejected (which happens if the user closes the directory picker without selecting a directory), we setIsSharingDirectory(false);. On the other hand, if the call to window.showDirectoryPicker() throws an error (as happens on browsers where this feature is not implemented, i.e.:Uncaught TypeError: window.showDirectoryPicker is not a function), we setDirectorySharingBrowserErr(true);.

Granted I'm making the assumption that those are the only two situations common enough to distinguish between, and we don't need to implement any finer grained handling within each block. In theory it's possible that there's some other reason that the promise might be rejected, and some other error that could be thrown, in which case the behavior may or may not be what we want.

setDirectorySharingBrowserErr(true);
}
};

return (
Expand Down
5 changes: 5 additions & 0 deletions packages/teleport/src/DesktopSession/useDesktopSession.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export default function useDesktopSession() {

const [canShareDirectory, setCanShareDirectory] = useState(false);
const [isSharingDirectory, setIsSharingDirectory] = useState(false);
const [directorySharingBrowserErr, setDirectorySharingBrowserErr] =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of 3 booleans, would it make more sense to have a directory sharing state? It could encode all of this info:

  • has permissions to share
  • is actively sharing
  • failed to share due to an error

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

useState(false);

const { username, desktopName, clusterId } = useParams<UrlDesktopParams>();

Expand Down Expand Up @@ -155,7 +157,10 @@ export default function useDesktopSession() {
setClipboardState,
canShareDirectory,
isSharingDirectory,
directorySharingBrowserErr,
setDirectorySharingBrowserErr,
setIsSharingDirectory,
isUsingChrome,
fetchAttempt,
tdpConnection,
wsConnection,
Expand Down