fix(pre-req): several fixes to the current hidden window launching process - INS-3319 #7174

ihexxa · 2024-03-13T06:48:56Z

Please review this PR (#7173) firstly as current one is based on it

Changes:

fix: the hidden window is not started in some launching paths (when app is packaged), so that sending request is always timeout as the hidden window is never up.
fix: restart the hidden window if it is down (just for defense)
test: updated one smoke test to test it
fix: restart the hidden window if it is stuck

How to test

Try to close the hidden window or make it crash and send a request.

packages/insomnia/src/hidden-window.ts

jackkav · 2024-03-15T08:50:37Z

Please clean up commits in this PR and address both detection and recovery.

…est is always timeout

jackkav · 2024-03-18T09:13:19Z

packages/insomnia-smoke-test/tests/smoke/pre-request-script-window.test.ts

+  // as the hidden window is restarted, it should not show "Timeout: Hidden browser window is not responding"
+  await page.getByText('Timeout: Pre-request script took too long').click();


jackkav · 2024-03-18T09:16:48Z

If i'm not mistaken this PR doesn't yet detect a a unresponsive or closed window it just tries to open the same window on every run and the promise will hang if the hidden window isn't up. Either way its important to make it very clear here since it we can't afford for this mechanism to be buggy.

ihexxa · 2024-03-18T09:21:54Z

Thanks for heads up, I've just added the mechanism to restart the stuck hidden window.

ihexxa · 2024-03-18T09:31:34Z

I would say that it should still be an advance as in the current implementation the hidden window seems never up. :(
Let's test it.

ihexxa · 2024-03-19T06:21:47Z

I would like to share 2 videos to help understand how does current window restarting mechanism work:

In this one, the hidden window is not started at beginning, it is started when send is clicked, the hidden window is started, for twice.
This means that the mechanism can start the hidden window if it is down.
https://github.com/Kong/insomnia/assets/24986718/60d4cc8a-93cc-4467-9814-2d149a4299aa

In this one, one infinite loop script is enabled. In the first sending, it hangs. And cancel the first one, disable infinite loop and re-trigger send, it still works.
This means that it can reset the hidden window to normal if it hangs.
https://github.com/Kong/insomnia/assets/24986718/2befb295-fb0d-4752-87c0-68640b28c4de

jackkav

In order to help move this PR forward and add value faster. I took the liberty of removing the new code paths and focused this PR in to only fixing the start hidden window bug. Lets address the retry mechanism in its own PR since it is not a trivial UX or implementation. I have left the commits in detailing the approach to syncing a busy flag in main file scoped variable. So we can check the trade-offs against other detection and retry mechanisms.

jackkav · 2024-03-20T17:47:48Z

packages/insomnia/src/main/window-utils.ts

-      stopHiddenBrowserWindow();
-    });
-  }
+export async function createHiddenBrowserWindow() {
  const hiddenBrowserWindow = new BrowserWindow({


since we check the browsers windows map outside of this function we no longer need to detect it being open and restart it. Since we should aim to trust the state of this map otherwise there is no benefit to keeping a record of its state in the map.

Let me try to clarify it. The state of window and the state of execution are 2 different states.

The windows map: means the hidden window is up or down.
The busy state: means if the hidden window is busy.
So, the hidden window is up != the hidden window is busy.

If the busy state is unknown, the mechanism must restart the window every time (to avoid it hangs), which seems not a good option for performance.
I can give more detailed elaboration of these 2 states if you would like to.

jackkav · 2024-03-20T17:50:16Z

packages/insomnia/src/main/window-utils.ts

+  const mainWindow = browserWindows.get('Insomnia') ?? createWindow();
+  if (!browserWindows.get('HiddenBrowserWindow')) {
+    createHiddenBrowserWindow();


it helps to keep createWindow and createHiddenWindow signatures and behaviour aligned.

…r aligned

ihexxa · 2024-03-21T09:53:18Z

Really thankful for the review, probably some points should be clarified to make sure that we are on the same page:

This PR basically a hotfix PR which fixes some showstopper bugs in the existing sandbox launching mechanism, so I would propose to have another PR when we've figured out a better solution (after comparison). Iteration normally is the better practice than making it perfect at first. :)
As discussed, the restarting action should be transparent to users, pls let me know if we need any more improvement on the ux.

Some tests are also failed after force-pushing, so changes are stored. Lets follow the review process in most of repos by adding comments instead of modifying them.

jackkav · 2024-03-21T11:31:59Z

packages/insomnia-smoke-test/tests/smoke/pre-request-script-window.test.ts

@@ -57,5 +60,6 @@ test('handle hidden browser window getting closed', async ({ app, page }) => {
  const hiddenWindow = windows[1];
  hiddenWindow.close();
  await page.getByRole('button', { name: 'Send' }).click();
-  await page.getByText('Timeout: Hidden browser window is not responding').click();
+  // as the hidden window is restarted, it should not show "Timeout: Hidden browser window is not responding"
+  await page.getByText('Timeout: Pre-request script took too long').click();


Suggested change

await page.getByText('Timeout: Pre-request script took too long').click();

await page.getByText('Timeout: An error occurred while running a pre-request script please restart Insomnia').click();

jackkav · 2024-03-21T11:32:48Z

packages/insomnia/src/main/window-utils.ts

+    ipcMain.removeHandler('renderer-listener-ready');
+    const hiddenWinListenerReady = new Promise<void>(resolve => {
+      ipcMain.handleOnce('renderer-listener-ready', () => {
+        console.log('[main] hidden window listener is ready');
+        resolve();
+      });
+    });
+    await hiddenWinListenerReady;
+
+    ipcMain.removeHandler('hidden-window-received-port');
+    const hiddenWinPortReady = new Promise<void>(resolve => {
+      ipcMain.handleOnce('hidden-window-received-port', () => {
+        console.log('[main] hidden window has received port');
+        resolve();
+      });
+    });
+
    const { port1, port2 } = new MessageChannelMain();
    hiddenBrowserWindow.webContents.postMessage('renderer-listener', null, [port1]);
+    await hiddenWinPortReady;
+
+    ipcMain.removeHandler('main-window-script-port-ready');
+    const mainWinPortReady = new Promise<void>(resolve => {
+      ipcMain.handleOnce('main-window-script-port-ready', () => {
+        console.log('[main] main window has received hidden window port');
+        resolve();
+      });
+    });


I don't see value in this. Can you explain why its required in this bug fix PR?

Value add: this is a defensive way to show a console log allowing a specific sync error to be deduced from app logs.

packages/insomnia/src/main/window-utils.ts

sentry-io · 2024-03-22T10:10:34Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ Error: Attempted to register a second handler for 'open-channel-to-hidden-browser-window' createWindow(src/main/window-utils) View Issue
‼️ Error: Attempted to register a second handler for 'open-channel-to-hidden-browser-window' createWindow(src/main/window-utils) View Issue

_{Did you find this useful? React with a 👍 or 👎}

ihexxa changed the title ~~chore: restart hidden window if it is down~~ fix: restart hidden window if it is down Mar 13, 2024

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

packages/insomnia/src/hidden-window.ts Fixed Show fixed Hide fixed

ihexxa self-assigned this Mar 13, 2024

ihexxa mentioned this pull request Mar 13, 2024

feat: pre-request sdk object cleanups - INS-3379 #7150

Closed

9 tasks

ihexxa added 3 commits March 15, 2024 22:31

fix: restart the hidden window if it is down

7d77d60

fix: lint error

ad39091

fix: smoke test error because of restarting hidden window

50b27b2

ihexxa force-pushed the pre-req-win-cleanup-5 branch from c511733 to 50b27b2 Compare March 15, 2024 14:36

ihexxa changed the title ~~fix: restart hidden window if it is down~~ fix: restart hidden window if it is down - INS-3319 Mar 15, 2024

ihexxa changed the title ~~fix: restart hidden window if it is down - INS-3319~~ fix(pre-req): restart hidden window if it is down - INS-3319 Mar 18, 2024

fix: the hidden window is not started after packaged, so sending requ…

a299dff

…est is always timeout

ihexxa changed the title ~~fix(pre-req): restart hidden window if it is down - INS-3319~~ fix(pre-req): several fixes to the current hidden window launching process - INS-3319 Mar 18, 2024

jackkav reviewed Mar 18, 2024

View reviewed changes

fix: restart the hidden window when it is stuck

8010fcf

jackkav force-pushed the pre-req-win-cleanup-5 branch 2 times, most recently from 9596351 to 1c53738 Compare March 20, 2024 17:36

jackkav previously approved these changes Mar 20, 2024

View reviewed changes

jackkav dismissed their stale review via 7577e51 March 20, 2024 17:44

jackkav force-pushed the pre-req-win-cleanup-5 branch from 7577e51 to f8c6d2a Compare March 20, 2024 17:44

jackkav reviewed Mar 20, 2024

View reviewed changes

ihexxa force-pushed the pre-req-win-cleanup-5 branch from f8c6d2a to 8010fcf Compare March 21, 2024 09:09

fix: make createWindow and createHiddenWindow signatures and behaviou…

6e81684

…r aligned

jackkav reviewed Mar 21, 2024

View reviewed changes

flatten conditions

ad4b84c

jackkav approved these changes Mar 21, 2024

View reviewed changes

ihexxa enabled auto-merge (squash) March 22, 2024 03:00

Merge branch 'develop' into pre-req-win-cleanup-5

cb8b4b8

ihexxa merged commit 915513c into develop Mar 22, 2024
7 checks passed

ihexxa deleted the pre-req-win-cleanup-5 branch March 22, 2024 03:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pre-req): several fixes to the current hidden window launching process - INS-3319 #7174

fix(pre-req): several fixes to the current hidden window launching process - INS-3319 #7174

ihexxa commented Mar 13, 2024 •

edited

jackkav commented Mar 15, 2024

jackkav Mar 18, 2024

jackkav commented Mar 18, 2024

ihexxa commented Mar 18, 2024

ihexxa commented Mar 18, 2024

ihexxa commented Mar 19, 2024 •

edited

jackkav left a comment

jackkav Mar 20, 2024

ihexxa Mar 21, 2024

jackkav Mar 20, 2024

ihexxa Mar 21, 2024

ihexxa commented Mar 21, 2024 •

edited

jackkav Mar 21, 2024

jackkav Mar 21, 2024

jackkav Mar 21, 2024

sentry-io bot commented Mar 22, 2024 •

edited

		// as the hidden window is restarted, it should not show "Timeout: Hidden browser window is not responding"
		await page.getByText('Timeout: Pre-request script took too long').click();

	await page.getByText('Timeout: Pre-request script took too long').click();
	await page.getByText('Timeout: An error occurred while running a pre-request script please restart Insomnia').click();

fix(pre-req): several fixes to the current hidden window launching process - INS-3319 #7174

fix(pre-req): several fixes to the current hidden window launching process - INS-3319 #7174

Conversation

ihexxa commented Mar 13, 2024 • edited

Please review this PR (#7173) firstly as current one is based on it

How to test

jackkav commented Mar 15, 2024

Choose a reason for hiding this comment

jackkav commented Mar 18, 2024

ihexxa commented Mar 18, 2024

ihexxa commented Mar 18, 2024

ihexxa commented Mar 19, 2024 • edited

jackkav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihexxa commented Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Mar 22, 2024 • edited

Suspect Issues

ihexxa commented Mar 13, 2024 •

edited

ihexxa commented Mar 19, 2024 •

edited

ihexxa commented Mar 21, 2024 •

edited

sentry-io bot commented Mar 22, 2024 •

edited