fix(dgw): fallback to recording playback when shadowing is not possible #1181

irvingoujAtDevolution · 2025-01-08T14:39:41Z

This happens when the session is already terminated.

Screen.Recording.2025-01-08.093650.mp4

github-actions · 2025-01-08T14:40:03Z

Let maintainers know that an action is required on their side

Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)
Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release
Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)
Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

webapp/player-project/src/main.ts

Co-authored-by: Benoît Cortier <[email protected]>

CBenoit · 2025-01-08T15:23:31Z

webapp/player-project/src/main.ts

+        showNotification('Session may have ended,playing recording instead', 'info');
+        return {
+          ...closeEvent,
+          code: 1000, // for avoid extra handling by other listeners


nitpick: I understand you are not used to it, but I would appreciate if you could follow this style more closely.
https://github.com/Devolutions/IronRDP/blob/master/STYLE.md#inline-code-comments-are-proper-sentences

The important thing with this suggestion is:

Rationale: writing a sentence (or maybe even a paragraph) rather just "a comment" creates a more appropriate frame of mind. It tricks you into writing down more of the context you keep in your head while coding.

At my level, I don’t really understand the connection between "1000" and "avoid extra handling by other listeners", suggesting the comment could give me more relevant context to understand what is happening. Of course, I could start digging up the code to try to understand why this is good or necessary, but it’s always nice when the comment can save more time.

updated , it is for asciinema-player, I added the link of the specific line that would cause problem in the comments

Did you forget to push your commit? I don’t see the change. Also, what do you mean by link? Since the codebase is evolving, a link to our GitHub repository may become outdated at anytime. Instead, describes using module name, function name, and other stable descriptions. It’s also okay to define anchors. For instance:

// ANCHOR: some_anchor_name let foo = bar;

// Search for the some_anchor_name anchor to see where this is relevant.

Sorry, my muscle memory makes switched me to another terminal without looking at the push result properly. I push the proper comment, and it is a link to the asciinema-player github project at the line where code 1000 is required.

I'll adjust accordingly.

Oh, it’s a different GitHub project? Then forget what I said, the link is fine!

CBenoit · 2025-01-08T15:29:06Z

Do you have a Jira ticket for this task? If so, add the footer: Issue: DGW-XXX in the PR description, and make sure it is included in the squash commit.

irvingoujAtDevolution · 2025-01-09T09:46:48Z

No, this is not related to any existing Jira tickets

…b.com:Devolutions/devolutions-gateway into proper-fall-back-when-streaming-not-available

CBenoit · 2025-01-09T16:16:07Z

webapp/player-project/src/main.ts

+          // for avoid extra handling by other listeners, for asciinema-player particularly in this case.
+          // see asciinema-player websocket driver at on socket close handler
+          code: 1000,


suggestion: Here is an improved wording. Both a stable description and a permalink are provided since this is an external repository.

Suggested change

// for avoid extra handling by other listeners, for asciinema-player particularly in this case.

// see asciinema-player websocket driver at on socket close handler

code: 1000,

// This prevents extra handling by other listeners, particularly for asciinema-player in this scenario.

// For more details, see the asciinema-player WebSocket driver’s socket close handler.

// https://github.com/asciinema/asciinema-player/blob/c09e1d2625450a32e9e76063cdc315fd54ecdd9d/src/driver/websocket.js#L219

code: 1000,

CBenoit · 2025-01-09T16:25:04Z

webapp/player-project/src/main.ts

@@ -23,6 +25,20 @@ async function playSessionShadowing(gatewayAccessApi) {
  try {
    const recordingInfo = await gatewayAccessApi.fetchRecordingInfo();
    const fileType = getFileType(recordingInfo);
+    BeforeWebsocketClose((closeEvent) => {
+      if (closeEvent.code !== 1000) {
+        // faild, try to play static recording


suggestion: This comment is also not clearly worded out. Here is how you could improve that.

Suggested change

// faild, try to play static recording

// The session playback failed; attempt to play the recording as usual as a fallback.

CBenoit · 2025-01-09T16:26:13Z

webapp/player-project/src/streamers/index.ts

@@ -17,3 +17,12 @@ export const getShadowPlayer = (fileType) => {

  return player;
 };
+
+export const cleanUpStreamers = () => {
+  // Clean up any existing shadow-player elements


suggestion: This comment is probably unnecessary, but if you want to keep it, here is a better wording.

Suggested change

// Clean up any existing shadow-player elements

// Remove all shadow-player elements.

Note that in this case, it’s much more useful to describe the why, rather than the what. Maybe it makes more sense to explain the why at the callsite of this function.
Here, the why could be something like "Removes the shadow player layout, so the regular recording player is visible again."
The reader of the code now understands the intention, and it’s easier to work from there.

CBenoit

I’m sorry for the pedantic comments, but it would really help me as a reviewer, and all future readers of the code, including yourself, if you could take some time to consider what I said carefully, and include the suggested practices in your coding style in your future contributions. I left and applied new comments, and I’ll leave you double check and merge the PR. If you struggle with English redaction, it’s fine to let an AI help you with that. ChatGPT or Copilot will do a great job at improving the text itself, but you need to write with intention. Thank you.

fix(dgw):properly fallback to recording when streaming is not available

1421eb8

Update biome.json

46365d6

irvingoujAtDevolution requested review from CBenoit and kristahouse January 8, 2025 14:43

CBenoit reviewed Jan 8, 2025

View reviewed changes

webapp/player-project/src/main.ts Outdated Show resolved Hide resolved

Update webapp/player-project/src/main.ts

642c95c

Co-authored-by: Benoît Cortier <[email protected]>

CBenoit reviewed Jan 8, 2025

View reviewed changes

CBenoit changed the title ~~fix(dgw):properly fallback to recording when streaming is not available~~ fix(dgw): fallback to recording playback when shadowing is not possible Jan 8, 2025

irvingoujAtDevolution added 5 commits January 9, 2025 04:51

review-fix

e4f6b28

remove review part

a063985

Merge branch 'proper-fall-back-when-streaming-not-available' of githu…

c3a3a98

…b.com:Devolutions/devolutions-gateway into proper-fall-back-when-streaming-not-available

remove unwanted change

b27f66b

review fix

bf052b8

CBenoit reviewed Jan 9, 2025

View reviewed changes

Update webapp/player-project/src/main.ts

7c87697

CBenoit reviewed Jan 9, 2025

View reviewed changes

CBenoit added 2 commits January 9, 2025 11:27

Update webapp/player-project/src/streamers/index.ts

3d3faa0

Update webapp/player-project/src/main.ts

38ac122

CBenoit approved these changes Jan 9, 2025

View reviewed changes

irvingoujAtDevolution enabled auto-merge (squash) January 9, 2025 16:44

irvingoujAtDevolution merged commit 8cb4c66 into master Jan 9, 2025
34 checks passed

irvingoujAtDevolution deleted the proper-fall-back-when-streaming-not-available branch January 9, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dgw): fallback to recording playback when shadowing is not possible #1181

fix(dgw): fallback to recording playback when shadowing is not possible #1181

irvingoujAtDevolution commented Jan 8, 2025 •

edited by CBenoit

Loading

github-actions bot commented Jan 8, 2025

CBenoit Jan 8, 2025 •

edited

Loading

irvingoujAtDevolution Jan 9, 2025

CBenoit Jan 9, 2025 •

edited

Loading

irvingoujAtDevolution Jan 9, 2025 •

edited

Loading

irvingoujAtDevolution Jan 9, 2025

CBenoit Jan 9, 2025

CBenoit commented Jan 8, 2025

irvingoujAtDevolution commented Jan 9, 2025

CBenoit Jan 9, 2025 •

edited

Loading

CBenoit Jan 9, 2025

CBenoit Jan 9, 2025

CBenoit Jan 9, 2025 •

edited

Loading

CBenoit left a comment •

edited

Loading

	// faild, try to play static recording
	// The session playback failed; attempt to play the recording as usual as a fallback.

	// Clean up any existing shadow-player elements
	// Remove all shadow-player elements.

fix(dgw): fallback to recording playback when shadowing is not possible #1181

fix(dgw): fallback to recording playback when shadowing is not possible #1181

Conversation

irvingoujAtDevolution commented Jan 8, 2025 • edited by CBenoit Loading

github-actions bot commented Jan 8, 2025

Let maintainers know that an action is required on their side

CBenoit Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

irvingoujAtDevolution Jan 9, 2025

Choose a reason for hiding this comment

CBenoit Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

irvingoujAtDevolution Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

irvingoujAtDevolution Jan 9, 2025

Choose a reason for hiding this comment

CBenoit Jan 9, 2025

Choose a reason for hiding this comment

CBenoit commented Jan 8, 2025

irvingoujAtDevolution commented Jan 9, 2025

CBenoit Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

CBenoit Jan 9, 2025

Choose a reason for hiding this comment

CBenoit Jan 9, 2025

Choose a reason for hiding this comment

CBenoit Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

CBenoit left a comment • edited Loading

Choose a reason for hiding this comment

irvingoujAtDevolution commented Jan 8, 2025 •

edited by CBenoit

Loading

CBenoit Jan 8, 2025 •

edited

Loading

CBenoit Jan 9, 2025 •

edited

Loading

irvingoujAtDevolution Jan 9, 2025 •

edited

Loading

CBenoit Jan 9, 2025 •

edited

Loading

CBenoit Jan 9, 2025 •

edited

Loading

CBenoit left a comment •

edited

Loading