Skip to content

Ensure sharing remains enabled if the initial desktop connection attempt fails#55350

Merged
gzdunek merged 4 commits intomasterfrom
gzdunek/allow-sharing-after-error
Jun 5, 2025
Merged

Ensure sharing remains enabled if the initial desktop connection attempt fails#55350
gzdunek merged 4 commits intomasterfrom
gzdunek/allow-sharing-after-error

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Jun 2, 2025

Currently, if the initial desktop connection fails (for example, because the desktop is not available), we reset the sharing state to its default. However, when the user clicks "Reconnect", the sharing state is not updated, so sharing remains disabled even if the connection succeeds.
I fixed that by resetting only the part that shows if we "currently" share something (which makes sense only for directory sharing, clipboard sharing is always active if ACL and the browser support it).

Generally, the entire state should be refactored, as we keep there things that shouldn't be there at all (like a flag if sharing is supported by the browser). I left a TODO for that.

changelog: Fixed a bug that caused clipboard and directory sharing to remain unavailable when the initial desktop connection failed

@gzdunek gzdunek added test-plan-problem Issues which have been surfaced by running the manual release test plan backport/branch/v16 backport/branch/v17 backport/branch/v18 labels Jun 2, 2025
Comment on lines -366 to -371
...prevState,
isSharing: false,
}));
setDirectorySharingState(prevState => ({
...prevState,
isSharing: false,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's no isSharing field in ClipboardSharingState/DirectorySharingState, so this did nothing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh my god I forgot that it's a thing in TypeScript. 🫩

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to write a regression test for that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added. In the end I had to refactor the state, because the hardcoded check for the user agent didn't allow me test this properly.

@ravicious ravicious self-requested a review June 2, 2025 10:50
Comment thread web/packages/teleport/src/DesktopSession/DesktopSession.tsx
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I verified that the issue was solved in Web UI and Connect and that in Firefox the app still correctly says that clipboard sharing is not supported in this browser.

Comment on lines -366 to -371
...prevState,
isSharing: false,
}));
setDirectorySharingState(prevState => ({
...prevState,
isSharing: false,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh my god I forgot that it's a thing in TypeScript. 🫩

@gzdunek gzdunek added this pull request to the merge queue Jun 5, 2025
Merged via the queue into master with commit 1562144 Jun 5, 2025
39 checks passed
@gzdunek gzdunek deleted the gzdunek/allow-sharing-after-error branch June 5, 2025 07:31
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@gzdunek See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Failed
branch/v18 Create PR

gzdunek added a commit that referenced this pull request Jun 5, 2025
…mpt fails (#55350)

* Ensure sharing remains enabled if the initial desktop connection attempt fails

* Refactor sharing state

* Add test

* Center "more actions" button vertically

(cherry picked from commit 1562144)
gzdunek added a commit that referenced this pull request Jun 5, 2025
…mpt fails (#55350)

* Ensure sharing remains enabled if the initial desktop connection attempt fails

* Refactor sharing state

* Add test

* Center "more actions" button vertically

(cherry picked from commit 1562144)
gzdunek added a commit that referenced this pull request Jun 5, 2025
…mpt fails (#55350)

* Ensure sharing remains enabled if the initial desktop connection attempt fails

* Refactor sharing state

* Add test

* Center "more actions" button vertically

(cherry picked from commit 1562144)
github-merge-queue Bot pushed a commit that referenced this pull request Jun 5, 2025
…mpt fails (#55350) (#55454)

* Ensure sharing remains enabled if the initial desktop connection attempt fails

* Refactor sharing state

* Add test

* Center "more actions" button vertically

(cherry picked from commit 1562144)
github-merge-queue Bot pushed a commit that referenced this pull request Jun 6, 2025
…n attempt fails (#55455)

* Ensure sharing remains enabled if the initial desktop connection attempt fails (#55350)

* Ensure sharing remains enabled if the initial desktop connection attempt fails

* Refactor sharing state

* Add test

* Center "more actions" button vertically

(cherry picked from commit 1562144)

* Remove test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 backport/branch/v18 size/sm test-plan-problem Issues which have been surfaced by running the manual release test plan ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants