Fix uncaught rangeerror by making manage logs/telemetry a page instead of dialog#14560
Fix uncaught rangeerror by making manage logs/telemetry a page instead of dialog#14560adamint wants to merge 1 commit intodotnet:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14560Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14560" |
There was a problem hiding this comment.
Pull request overview
This PR resolves a FluentUI-Blazor nested-dialog focus recursion issue (manifesting as an uncaught RangeError) by replacing the “Manage logs and telemetry” dialog with a dedicated /managedata page and updating Settings to navigate to it.
Changes:
- Added a new
ManageDatapage (Razor + code-behind + isolated CSS) at/managedata. - Updated Settings dialog to navigate to the new page instead of opening a nested dialog.
- Added a
DashboardUrls.ManageDataUrl()helper and removed the oldManageDataDialog.razor.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/DashboardUrls.cs | Adds a /managedata URL helper for consistent navigation. |
| src/Aspire.Dashboard/Components/Pages/ManageData.razor | Introduces the new Manage Data page route and UI markup. |
| src/Aspire.Dashboard/Components/Pages/ManageData.razor.cs | Moves dialog logic to a page component and updates localization type usage. |
| src/Aspire.Dashboard/Components/Pages/ManageData.razor.css | Adjusts layout so grid scrolls while action buttons stay pinned at the bottom. |
| src/Aspire.Dashboard/Components/Dialogs/SettingsDialog.razor | Updates the Manage button click handler to navigate to the page. |
| src/Aspire.Dashboard/Components/Dialogs/SettingsDialog.razor.cs | Replaces nested dialog launch with navigation to /managedata. |
| src/Aspire.Dashboard/Components/Dialogs/ManageDataDialog.razor | Removes the obsolete dialog UI. |
Comments suppressed due to low confidence (1)
src/Aspire.Dashboard/Components/Dialogs/SettingsDialog.razor:43
- There are existing Playwright tests covering the Settings dialog (e.g., theme changes). Since this change is specifically fixing a regression triggered by the Manage button (nested dialog focus recursion / RangeError), it would be good to add an automated regression test that clicks this button and verifies navigation to
/managedata(and ideally asserts no console errors are emitted).
<FluentButton IconStart="@(new Icons.Regular.Size16.Settings())"
OnClick="NavigateToManageData"
Title="@Loc[nameof(Dialogs.ManageDataManageButtonText)]"
aria-label="@Loc[nameof(Dialogs.ManageDataManageButtonText)]">
@Loc[nameof(Dialogs.ManageDataManageButtonText)]
</FluentButton>
| public const string ManageDataBasePath = "managedata"; | ||
|
|
||
| public static string ManageDataUrl() => $"/{ManageDataBasePath}"; |
There was a problem hiding this comment.
A new URL helper was added here; the repo already has DashboardUrlsTests covering several URL builders. Consider adding a small unit test for ManageDataUrl() (and/or ManageDataBasePath) to keep coverage consistent and catch accidental route changes.
| private async void NavigateToManageData() | ||
| { | ||
| var parameters = new DialogParameters | ||
| { | ||
| Title = Loc[nameof(Dashboard.Resources.Dialogs.ManageDataDialogTitle)], | ||
| PrimaryAction = Loc[nameof(Dashboard.Resources.Dialogs.DialogCloseButtonText)], | ||
| SecondaryAction = string.Empty, | ||
| Width = "800px", | ||
| Height = "auto" | ||
| }; | ||
| await DialogService.ShowDialogAsync<ManageDataDialog>(parameters); | ||
| NavigationManager.NavigateTo(DashboardUrls.ManageDataUrl()); | ||
| } |
There was a problem hiding this comment.
NavigateToManageData is declared as async void but contains no await, which will produce CS1998 (and this repo treats warnings as errors). Also, since the settings dialog is tracked by MainLayout (_openPageDialog) and there’s no route-change handler to auto-close it, navigating here will leave the settings panel/dialog open over the destination page. Make this a Task-returning handler and close the current FluentDialog (via a CascadingParameter like other dialogs) before calling NavigationManager.NavigateTo(...).
Description
From the issue:
This PR creates a new page and removes the old dialog. The bottom buttons always appear on the bottom of the screen.
Fixes #14407
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: