Skip to content

Simplify TDP connection state#53233

Merged
gzdunek merged 4 commits intomasterfrom
gzdunek/simplify-connection-state
Mar 21, 2025
Merged

Simplify TDP connection state#53233
gzdunek merged 4 commits intomasterfrom
gzdunek/simplify-connection-state

Conversation

@gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Mar 20, 2025

The primary goal of this PR is to simplify nextScreenState, which was previously unnecessarily complicated.
Additionally, the previous implementation used useState to manage this state and updated it inside a useEffect, which is a React anti-pattern.

To improve this, wsConnection and tdpConnection attempts were combined into a single tdpConnectionStatus which has the following states:

type TdpConnectionStatus =
  /** Unknown status.*/
  | { status: '' }
  /** The transport connection has been established.*/
  | { status: 'connected' }
  /** The remote desktop is visible as we received the first frame. */
  | { status: 'active' }
  /**
   * The client has disconnected.
   * This can happen either gracefully (on the remote side)
   * or due to closing the transport layer.
   */
  | {
      status: 'disconnected';
      message: string;
    };

Note that we don't keep information about graceful/non-graceful disconnection, everything is mapped to a generic "disconnected" status with a message. Why? Because the UI treats all disconnections the same, displaying the same component regardless of the reason.

I recommend reviewing commit by commit.

PS: This is still not the final version of this component. I’m opening smaller PRs to keep the review process manageable.

@gzdunek gzdunek added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 backport/branch/v17 labels Mar 20, 2025
@gzdunek gzdunek requested review from avatus and zmb3 March 20, 2025 15:04
@github-actions github-actions bot requested review from kiosion and ravicious March 20, 2025 15:05
/>
);

export const TdpProcessing = () => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had stories that looked the same, I removed them.

@gzdunek gzdunek removed request for kiosion and ravicious March 20, 2025 15:07
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

I'll try to run it a bit later.

Comment on lines +763 to 766
resize = (spec: ClientScreenSpec) => {
this.sendClientScreenSpec(spec);
}
};

Copy link
Contributor

@avatus avatus Mar 21, 2025

Choose a reason for hiding this comment

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

how was it working before without the lexical binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was wrapped in an arrow function:

  const onResize = (e: { height: number; width: number }) => {
     tdpClient.resize(e);
   };

I removed it and called the function directly.

@gzdunek gzdunek enabled auto-merge March 21, 2025 14:42
@gzdunek gzdunek added this pull request to the merge queue Mar 21, 2025
Merged via the queue into master with commit 4780986 Mar 21, 2025
40 checks passed
@gzdunek gzdunek deleted the gzdunek/simplify-connection-state branch March 21, 2025 15:02
@backport-bot-workflows
Copy link
Contributor

@gzdunek See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Create PR

gzdunek added a commit that referenced this pull request Mar 21, 2025
* Continue removing `useTdpClientCanvas`

* Simplify connection state and `nextScreenState`

* Call `initWasm` only once

* Do not overwrite the keyboard handler

(cherry picked from commit 4780986)
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2025
* Continue removing `useTdpClientCanvas`

* Simplify connection state and `nextScreenState`

* Call `initWasm` only once

* Do not overwrite the keyboard handler

(cherry picked from commit 4780986)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants