fix: keep the active tab when a tab close is cancelled - #157
Open
jafin wants to merge 2 commits into
Open
Conversation
Dismissing the "are you sure you want to close" prompt left the tab open but switched the active connection to the tab on its left. DockPanelSuite's TryCloseTab calls SelectClosestPane straight after DockPane.CloseContent without checking whether the content actually closed, and it sets ActiveContent to Tabs[index - 1]. EnableSelectClosestOnClose defaults to true and connDock uses DocumentStyle.DockingWindow whenever more than one tab is open, so both of its guards pass. The selection therefore moved even though ConnectionTab.OnFormClosing had cancelled the close, which is why it only showed up on tabs other than the first. Wrap the close so the previous selection is restored when the tab is still displaying afterwards. Keying off DisplayingContents rather than a cancel flag covers every refusal - the confirmation prompt, a protocol veto, KeepTabsOpenAfterDisconnect - and leaves a successful close alone.
PR Summary by QodoFix cancelled tab-close changing the active connection tab
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
There was a problem hiding this comment.
Pull request overview
Fixes a UI regression where cancelling a connection tab close (e.g., dismissing the “are you sure” prompt) leaves the tab open but incorrectly shifts the active selection to the adjacent tab, by restoring the prior active content when the close is refused.
Changes:
- Wrap DockPanelSuite’s
TryCloseTabin aCloseTabhelper that re-activates the previously active content when the close attempt doesn’t actually remove the tab. - Route queued tab-close actions through the new
CloseTabhelper to cover the existing close entry points. - Add an NUnit UI test that simulates a cancelled close (via
FormClosingcancellation) and asserts the active tab remains unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mRemoteNG/UI/Tabs/DockPaneStripNG.cs | Adds CloseTab(int) wrapper around TryCloseTab to restore selection when close is refused. |
| mRemoteNGTests/UI/Tabs/DockPaneStripNGTests.cs | Adds regression test ensuring cancelling a tab close keeps the active tab selected. |
Comment on lines
+165
to
+170
| DateTime start = DateTime.Now; | ||
| while ((DateTime.Now - start).TotalSeconds < 2) | ||
| { | ||
| Application.DoEvents(); | ||
| Thread.Sleep(10); | ||
| } |
Code Review by Qodo
1. Unconditional 2s test wait
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+134
to
+136
| hostForm.Controls.Add(dockPanel); | ||
| hostForm.Show(); | ||
|
|
There was a problem hiding this comment.
1. hostform.show() in test 📘 Rule violation ▣ Testability
The new test opens a real WinForms window via hostForm.Show(), which can create interactive UI during automated runs and cause flakiness in headless/CI environments. This violates the rule to avoid interactive UI calls in automated tests.
Agent Prompt
## Issue description
The new NUnit test calls `hostForm.Show()`, which opens a real WinForms window during automated test execution.
## Issue Context
Compliance requires automated tests to avoid real interactive UI calls. This test can likely be made non-interactive by forcing handle creation without showing the form (e.g., using `Handle`/`CreateControl()`), or by refactoring to test the behavior without constructing/showing real WinForms windows.
## Fix Focus Areas
- mRemoteNGTests/UI/Tabs/DockPaneStripNGTests.cs[134-170]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+166
to
+169
| while ((DateTime.Now - start).TotalSeconds < 2) | ||
| { | ||
| Application.DoEvents(); | ||
| Thread.Sleep(10); |
There was a problem hiding this comment.
2. Unconditional 2s test wait 🐞 Bug ☼ Reliability
The new test CancellingATabClose_LeavesTheActiveTabUnchanged always spins the message pump for the full 2 seconds with no early-exit condition, adding a fixed delay to every run and making the test more timeout-sensitive under load. This is avoidable because the test can poll for the expected stable state and break immediately once reached (as the existing test in the same file already does).
Agent Prompt
### Issue description
The new NUnit test uses a fixed 2-second `while` loop that always runs to completion, regardless of whether the UI work (`BeginInvoke` from the tab close path) has already completed. This unnecessarily slows the test suite and can cause intermittent failures if the expected state is not reached before the hardcoded timeout.
### Issue Context
The existing test `MiddleClick_ClosesSpecificTab_NotAll` already uses a better pattern: it pumps events but breaks early once the expected condition is satisfied.
### Fix Focus Areas
- mRemoteNGTests/UI/Tabs/DockPaneStripNGTests.cs[165-175]
### Suggested fix
- Replace the unconditional 2-second loop with a bounded polling loop that:
- Calls `Application.DoEvents()`
- Breaks early once both are true:
- `doc2.DockState == DockState.Document`
- `doc2.DockHandler.Pane.ActiveContent == doc2`
- Fails with a clear message if the condition is not met within the timeout.
- Prefer `Stopwatch` over `DateTime.Now` for measuring elapsed time.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…le timeout Thread.Interrupt only unblocks interruptible waits, so a wedged UI thread survived the timeout, and because it was a foreground thread it could hold the whole test run open. The test body is now posted onto an STA thread running Application.Run(new ApplicationContext()) and calls Application.ExitThread in a finally, so the loop unwinds as soon as the body finishes. On timeout the loop is asked to exit and joined again briefly before failing. The thread is also marked IsBackground as a last-resort net: even if it never pumps again it cannot keep the process alive. This helper is shared by the whole fixture, so MiddleClick_ClosesSpecificTab_NotAll gets the same treatment.
Author
|
Carried over a review fix from the upstream counterpart (mRemoteNG#3408):
The helper is shared by the whole fixture, so the pre-existing The helper's timeout path itself was verified separately with a standalone copy at a 2s timeout: a body of Two notes for anyone copying the helper: it uses |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closing a connection tab shows the "are you sure you want to close" prompt. Clicking Cancel correctly keeps the tab open, but the active connection switches to the tab on its left.
Root cause
Not in the confirmation logic — it is DockPanelSuite.
DockPaneStripBase.TryCloseTab:SelectClosestPaneruns unconditionally — it never checks whether the close actually happened.EnableSelectClosestOnClosedefaults totrue, andconnDock.DocumentStyleisDockingWindowwhenever more than one tab is open, so both of its guards pass. WhenConnectionTab.OnFormClosingsetse.Cancel = true, the tab survives but DPS has already moved the selection.It only bites when the tab being closed is not the first one (
SelectClosestPaneis guarded byindex > 0), which is why the bug can look intermittent.Fix
DockPaneStripNGwraps the close and puts the selection back when the tab is still on display:Keying off
DisplayingContentsrather than a cancel flag covers every refusal — the confirmation prompt, a protocol veto,KeepTabsOpenAfterDisconnect— and it deliberately does nothing on a successful close, so the existing MRU/adjacent-tab behaviour is untouched.Activate()rather than a bareActiveContentassignment also restores keyboard focus to the tab. Both entry points intoQueueCloseTab(close button and middle-click) are covered.Verification
Solution builds clean.
The NUnit suite could not be executed on my machine — every group reports
NUnit couldn't run the N discovered tests: Only supported on Windows10.0.26100.0, becausemRemoteNGTests.csprojsetsSupportedOSPlatformVersion 10.0.26100.0while the host is Windows 10.0.22631. Pre-existing and unrelated to this change; CI should run the suite normally.Behaviour was verified by driving the real
DockPaneStripNGfrom a standalone STA harness against the built assembly:The control line is the bug reproduced directly: calling DPS's
TryCloseTableaves the tab open but hands the selection toDoc1.New test:
DockPaneStripNGTests.CancellingATabClose_LeavesTheActiveTabUnchanged, using aFormClosinghandler that cancels — headless, no dialog, following the existing message-pump pattern in that fixture.