Repository navigation
Add the same menu items for subscriptions as for queues - #880
Conversation
|
@claude, do a review of this handsome pr |
|
Claude finished @ErikMogensen's task in 2m 7s —— View job PR Review: Add the same menu items for subscriptions as for queues
Overall this is a clean, well-scoped PR that achieves its goal. The renames are sensible, the new code follows the established patterns in Observations1. Missing "Select All" in the subscription deadletter context menu
2. Indentation inconsistency at end of The last two field declarations and the closing braces have incorrect indentation relative to the rest of the file: // Current (incorrectly indented)
private System.Windows.Forms.PropertyGrid deadletterCustomPropertyGrid;
private System.Windows.Forms.ToolStripMenuItem resubmitDeadletterToolStripMenuItem;
private System.Windows.Forms.ToolStripMenuItem resubmitMessageToolStripMenuItem;
}
}
3. In the subscription messages menu, the new item is ordered as: But in Minor UX inconsistency between the two controls' menus. 4. Pre-existing filename splitting bug (carried forward, not introduced here) Both the queue and subscription multi-save handlers use this pattern: // HandleSubscriptionControl.cs ~line 2953
var fileNameParts = saveFileDialog.FileName.Split('.').ToList();
var fileExtension = fileNameParts.Last();
fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); // BUG: finds first match, not last
What's Good
|
There was a problem hiding this comment.
Pull request overview
This PR aligns the subscription (including subscription DLQ) message grid context menus with the queue message grid by adding missing actions (notably “save message text as file” and “select all”), and it also renames several queue deadletter menu item variables to drop the misleading “shared” wording.
Changes:
- Add context-menu actions in
HandleSubscriptionControlto save selected message body text to file (single + multi) and to select all messages. - Add corresponding ToolStrip menu items/wiring in the subscription control designer.
- Rename queue deadletter context menu strip + menu item fields to remove “Shared” from variable names.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ServiceBusExplorer/Controls/HandleSubscriptionControl.cs |
Adds “save body as file” (single/multi) for messages and DLQ messages; adds “select all messages”; updates context-menu visibility logic. |
src/ServiceBusExplorer/Controls/HandleSubscriptionControl.Designer.cs |
Adds ToolStrip items and click wiring for the new subscription context-menu actions. |
src/ServiceBusExplorer/Controls/HandleQueueControl.cs |
Updates deadletter menu visibility wiring to use renamed (non-“shared”) menu item fields and context menu strip. |
src/ServiceBusExplorer/Controls/HandleQueueControl.Designer.cs |
Renames deadletter context menu strip + menu item fields; updates designer wiring accordingly. |
Files not reviewed (2)
- src/ServiceBusExplorer/Controls/HandleQueueControl.Designer.cs: Language not supported
- src/ServiceBusExplorer/Controls/HandleSubscriptionControl.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var body in bodies) | ||
| { | ||
| count++; | ||
| var fileNameParts = saveFileDialog.FileName.Split('.').ToList(); | ||
| var fileExtension = fileNameParts.Last(); | ||
| fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); | ||
| fileNameParts.Add($"({count}).{fileExtension}"); | ||
| var fileName = string.Join(".", fileNameParts); |
There was a problem hiding this comment.
The numbered-output filename generation is fragile: splitting the full path on '.' and removing the extension via IndexOf can remove the wrong segment when the filename contains multiple dots (e.g. "foo.txt.v1.txt") or the extension token appears earlier. Use Path.GetFileNameWithoutExtension/Path.GetExtension (or RemoveAt(fileNameParts.Count-1)) to build "{base}({count}){ext}" safely, preserving the directory portion.
| foreach (var body in bodies) | |
| { | |
| count++; | |
| var fileNameParts = saveFileDialog.FileName.Split('.').ToList(); | |
| var fileExtension = fileNameParts.Last(); | |
| fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); | |
| fileNameParts.Add($"({count}).{fileExtension}"); | |
| var fileName = string.Join(".", fileNameParts); | |
| var selectedFileName = saveFileDialog.FileName; | |
| var directoryName = Path.GetDirectoryName(selectedFileName); | |
| var baseFileName = Path.GetFileNameWithoutExtension(selectedFileName); | |
| var fileExtension = Path.GetExtension(selectedFileName); | |
| foreach (var body in bodies) | |
| { | |
| count++; | |
| var numberedFileName = $"{baseFileName}({count}){fileExtension}"; | |
| var fileName = string.IsNullOrEmpty(directoryName) | |
| ? numberedFileName | |
| : Path.Combine(directoryName, numberedFileName); |
| var fileNameParts = saveFileDialog.FileName.Split('.').ToList(); | ||
| var fileExtension = fileNameParts.Last(); | ||
| fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); | ||
| fileNameParts.Add($"({count}).{fileExtension}"); | ||
| var fileName = string.Join(".", fileNameParts); |
There was a problem hiding this comment.
Same issue here as in the messages bulk-save: generating numbered filenames by splitting on '.' and removing via IndexOf can produce incorrect paths when the chosen filename includes multiple dots. Prefer Path-based filename manipulation (GetDirectoryName + GetFileNameWithoutExtension + GetExtension) or at least remove the last segment rather than the first match.
| var fileNameParts = saveFileDialog.FileName.Split('.').ToList(); | |
| var fileExtension = fileNameParts.Last(); | |
| fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); | |
| fileNameParts.Add($"({count}).{fileExtension}"); | |
| var fileName = string.Join(".", fileNameParts); | |
| var directoryName = Path.GetDirectoryName(saveFileDialog.FileName); | |
| var fileNameWithoutExtension = Path.GetFileNameWithoutExtension(saveFileDialog.FileName); | |
| var fileExtension = Path.GetExtension(saveFileDialog.FileName); | |
| var numberedFileName = $"{fileNameWithoutExtension}({count}){fileExtension}"; | |
| var fileName = string.IsNullOrEmpty(directoryName) | |
| ? numberedFileName | |
| : Path.Combine(directoryName, numberedFileName); |
| // saveSelectedDeadletteredMessageToolStripMenuItem | ||
| // | ||
| this.saveSelectedSharedDeadletteredMessageToolStripMenuItem.Name = "saveSelectedSharedDeadletteredMessageToolStripMenuItem"; | ||
| this.saveSelectedSharedDeadletteredMessageToolStripMenuItem.Size = new System.Drawing.Size(305, 22); | ||
| this.saveSelectedSharedDeadletteredMessageToolStripMenuItem.Text = "Save Selected Message"; | ||
| this.saveSelectedSharedDeadletteredMessageToolStripMenuItem.Click += new System.EventHandler(this.saveSelectedSharedDeadletteredMessageToolStripMenuItem_Click); | ||
| this.saveSelectedDeadletteredMessageToolStripMenuItem.Name = "saveSelectedDeadletteredMessageToolStripMenuItem"; | ||
| this.saveSelectedDeadletteredMessageToolStripMenuItem.Size = new System.Drawing.Size(305, 22); | ||
| this.saveSelectedDeadletteredMessageToolStripMenuItem.Text = "Save Selected Message"; | ||
| this.saveSelectedDeadletteredMessageToolStripMenuItem.Click += new System.EventHandler(this.saveSelectedSharedDeadletteredMessageToolStripMenuItem_Click); | ||
| // | ||
| // saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem | ||
| // saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem | ||
| // | ||
| this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem.Name = "saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem"; | ||
| this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem.Size = new System.Drawing.Size(305, 22); | ||
| this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem.Text = "Save Selected Message Text as File"; | ||
| this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem.Click += new System.EventHandler(this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem_Click); | ||
| this.saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem.Name = "saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem"; | ||
| this.saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem.Size = new System.Drawing.Size(305, 22); | ||
| this.saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem.Text = "Save Selected Message Text as File"; | ||
| this.saveSelectedDeadletteredMessageBodyAsFileToolStripMenuItem.Click += new System.EventHandler(this.saveSelectedSharedDeadletteredMessageBodyAsFileToolStripMenuItem_Click); |
There was a problem hiding this comment.
These menu items were renamed to remove the "Shared" wording, but their Click handlers still point to methods named SharedDeadletter (e.g. saveSelectedSharedDeadletteredMessageToolStripMenuItem_Click). This mismatch makes it harder to understand/refactor the UI wiring; consider renaming the handler methods (and updating references) to match the new menu item names.
|
Looks like good feedback. I'll take a deeper look at it and update this PR later. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.
Files not reviewed (2)
- src/ServiceBusExplorer/Controls/HandleQueueControl.Designer.cs: Generated file
- src/ServiceBusExplorer/Controls/HandleSubscriptionControl.Designer.cs: Generated file
| var fileNameParts = saveFileDialog.FileName.Split('.').ToList(); | ||
| var fileExtension = fileNameParts.Last(); | ||
| fileNameParts.RemoveAt(fileNameParts.IndexOf(fileExtension)); | ||
| fileNameParts.Add($"({count}).{fileExtension}"); | ||
| var fileName = string.Join(".", fileNameParts); |
There was a problem hiding this comment.
This is an old issue, but I'll fix it.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Finally this is now ready for review. |
paolosalvatori
left a comment
paolosalvatori
left a comment
There was a problem hiding this comment.
Review summary
Overall assessment: Solid parity work. The subscription (and subscription DLQ) message grids now offer the same actions as the queue grids: save-body-as-file (single + multi), and Select All in both context menus. I checked the new handlers line-by-line against their HandleQueueControl counterparts — the right grid, binding source, and text box are used in each one (txtMessageText/messagesDataGridView for the messages menu, txtDeadletterText/deadletterDataGridView for the dead-letter menu). Menu item ordering now mirrors the queue's corresponding menus exactly, including Select All placement. The PathHelper.GetNumberedFileName extraction is the standout: it fixes the long-standing first-match-removal filename bug at all four call sites (both controls — the queue's dead-letter save paths route through SaveSelectedMessages, so they're covered too), and no Split('.') occurrences remain in either control. Utilities.csproj is SDK-style, so the new file is picked up, and both controls already import ServiceBusExplorer.Utilities.Helpers. Build and unit tests pass on CI.
Key risks:
- Merge-order coordination with #886. Both PRs edit the same lines of
HandleQueueControl.Designer.csfrom the same base, with different content (this PR keeps the old*Shared*handler names in theClickhookups and renamessharedDeadletterContextMenuStrip; #886 renames the handlers but keeps the strip name). I verified locally: merging this PR into main is clean, but merging #886 afterwards conflicts inHandleQueueControl.Designer.cs(either order conflicts). Recommendation: land this PR first, then rebase #886 and resolve — the resolution is mechanical (keep #886's handler renames + this PR's strip rename). - Nothing else of substance — the change is UI-only, no Service Bus API behavior is touched.
Status of prior review conversations (verdicts per thread):
- Copilot 2026-04-28 ·
HandleSubscriptionControl.cs:2957(fragile filename split) — never replied to, left unresolved, but fully addressed in code by 27cf790 ("Fixed old path issue" →PathHelper). Obsolete → safe to close. - Copilot 2026-04-28 ·
HandleSubscriptionControl.cs:3136(same issue, second site) — never replied to, left unresolved, addressed by 27cf790. Obsolete → safe to close. - Copilot 2026-04-28 ·
HandleQueueControl.Designer.cs:2238(menu items renamed but handlers still*Shared*) — never replied to, left unresolved. Deliberately deferred to #886 (per your reply on the newer duplicate thread), so: still valid until #886 lands; suggest replying with the #886 pointer and closing, like its duplicate. - Copilot 2026-07-14 ·
HandleSubscriptionControl.cs:2962— replied "I'll fix it", fixed by 27cf790, but left unresolved → safe to close. - Copilot 2026-07-14 ·
HandleSubscriptionControl.cs:3141— fixed by 27cf790, resolved ✓. - Copilot 2026-07-14 · designer-file tail indentation — fixed (verified the file tail on the branch), resolved ✓.
- Copilot 2026-07-14 ·
HandleQueueControl.Designer.cs:2232— resolved with "handled in #886"; accurate for the six save/delete handler renames ✓ (contingent on #886 actually landing). - Copilot 2026-07-14 ·
HandleQueueControl.cs:4384(renameShowAppropriateSharedDeadletterMenuItems) — resolved with "handled in #886", but #886 does not rename that method (norRepairAndResubmitSharedDeadletterMessageor the repair/resubmit/select-all*Shared*items). Needs author action: extend #886 or reopen/track this one.
From the claude[bot] issue-comment review of 2026-04-28 (never replied to, but all four points were addressed by later commits): missing Select All in the sub dead-letter menu → added in bd1ee3c ✓; designer indentation → fixed ✓; menu ordering → now mirrors the queue's messages menu ✓; filename bug → fixed via PathHelper ✓. Housekeeping: the 2026-07-17 @claude review once run errored without posting (actions run 29563797543).
Recommendation: Comment — effectively approve once (a) the four stale-but-fixed Copilot threads (items 1, 2, 3, 4 above) get a reply/close, and (b) the #886 merge order is agreed. Inline notes below are minor/nit.
| { | ||
| public static class PathHelper | ||
| { | ||
| public static string GetNumberedFileName(string fileName, int count) |
There was a problem hiding this comment.
minor (test coverage) — good extraction: Path-API based, fixes the first-match-removal bug Copilot flagged twice (report.txt.backup.txt now numbers correctly). This is the one pure, dependency-free unit in the PR and the repo already has ServiceBusExplorer.Tests with helper tests — please add a small test locking in exactly the cases the review threads were about:
[Test]
public void GetNumberedFileName_HandlesMultiDotNames() =>
Assert.AreEqual(@"C:\tmp\report.txt.backup(2).txt",
PathHelper.GetNumberedFileName(@"C:\tmp\report.txt.backup.txt", 2));plus a name without an extension ("file" → "file(1)") and a bare relative name (Path.GetDirectoryName returns "" there — the ?? string.Empty only matters for root paths).
| // deleteSelectedDeadletterMessageToolStripMenuItem | ||
| // | ||
| this.deleteSelectedMessageToolStripMenuItem.Name = "deleteSelectedSharedDeadletterMessageToolStripMenuItem"; | ||
| this.deleteSelectedMessageToolStripMenuItem.Name = "deleteSelectedDeadletterMessageToolStripMenuItem"; |
There was a problem hiding this comment.
nit — the .Name string and the comment header now say deleteSelectedDeadletterMessageToolStripMenuItem, but the field is still deleteSelectedMessageToolStripMenuItem (same for the plural one at line 1783). The mismatch predates this PR, but since the point of the rename pass is aligning identifiers, consider renaming the two fields to match. The WinForms designer treats Name as the component's serialization identity, and keeping field ↔ Name aligned avoids confusion the next time the designer regenerates this file.
| private System.Windows.Forms.ToolStripMenuItem repairAndResubmitMessageToolStripMenuItem; | ||
| private System.Windows.Forms.ToolStripMenuItem resubmitSelectedMessagesInBatchModeToolStripMenuItem; | ||
| private System.Windows.Forms.ContextMenuStrip sharedDeadletterContextMenuStrip; | ||
| private System.Windows.Forms.ContextMenuStrip deadletterContextMenuStrip; |
There was a problem hiding this comment.
nit — the sharedDeadletterContextMenuStrip → deadletterContextMenuStrip rename didn't reach HandleQueueControl.resx, which still carries
Also noting for other readers: the Click hookups on the renamed items above intentionally still point at the old *Shared* handler names — that's what #886 fixes; see the summary comment about merge order.
| foreach (var body in bodies) | ||
| { | ||
| count++; | ||
| var fileName = PathHelper.GetNumberedFileName(saveFileDialog.FileName, count); |
There was a problem hiding this comment.
nit (inherited behavior, for the follow-up refactor) — the SaveFileDialog's overwrite prompt covers foo.txt, but the loop actually writes foo(1).txt, foo(2).txt, … and silently File.Deletes any existing files with those names. Identical to the queue control, so consistent here — flagging it so the planned dedup PR can decide whether numbered targets deserve their own overwrite check.
paolosalvatori
left a comment
paolosalvatori
left a comment
There was a problem hiding this comment.
Hi Erik,
Thanks for this PR and for your patience with the long review cycle on it — the subscription grids finally behaving like the queue grids is something users have been missing for a while, and the PathHelper extraction quietly fixes that old filename bug everywhere, which I really appreciate.
I approve this PR, but please take a look at the comments I left this morning and make sure to implement them. To be explicit about what I'm asking:
-
Add a few unit tests for
PathHelper.GetNumberedFileNameinServiceBusExplorer.Tests. It's a pure function, so it's cheap to test, and it's exactly the thing the two Copilot threads complained about — I'd like the fix locked in. The cases I care about: a multi-dot name (report.txt.backup.txt→report.txt.backup(2).txt), a name without an extension, and a bare relative filename. -
Fix the leftover in
HandleQueueControl.resx— line 238 still sayssharedDeadletterContextMenuStrip.TrayLocation; please rename it to match the newdeadletterContextMenuStripname so the designer metadata stays consistent. -
Small one, your call: in
HandleSubscriptionControl.Designer.csthe delete items'.Namestrings now saydeleteSelectedDeadletterMessage(s)ToolStripMenuItembut the fields are stilldeleteSelectedMessage(s)ToolStripMenuItem. Since you're normalizing names anyway, aligning the fields too would avoid designer surprises later. -
Please close the review threads that are already fixed — the two April threads and the July one about the filename split are all addressed by your
27cf790commit but still show as open, and the April thread about the handler names just needs the same "#886" reply you gave its July duplicate. It'll make the PR page much easier to read.
One important coordination point: this PR needs to merge before #886. They touch the same lines in HandleQueueControl.Designer.cs and whichever lands second will conflict — I verified it locally. So let's land this one, then rebase #886 on top.
The overwrite-prompt quirk on the numbered files I flagged inline is inherited from the queue control — totally fine to leave for your planned dedup refactor.
Thanks again, great work on this one!
Paolo
|
Thanks @paolosalvatori! I don't know how you set it up, but impressive review! I'll merge this one and add unit tests. |
|
@ErikMogensen I used Claude Code to review PRs 😅 |
There are several menu items that are missing from the message grid for dead-letter queue messages in subscriptions. This PR brings makes them available for subscriptions too.
Removed the word 'shared' from some menu items variable names since they weren't shared.
This causes some code duplication, but that will be handled in a separate PR.