Skip to content

Remember resource filter states #8593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

adamint
Copy link
Member

@adamint adamint commented Apr 7, 2025

Description

Remembers the disabled states in this page. Moves string escape/unescape functions to StringUtils.

Fixes #3647

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Aspire.Dashboard/Utils/DashboardUrls.cs:19

  • The URL base no longer starts with a leading slash as it used to ($"/{ResourcesBasePath}"). This may cause issues with URL resolution; consider reintroducing the '/' prefix if required for proper routing.
var url = ResourcesBasePath;

@JamesNK
Copy link
Member

JamesNK commented Apr 9, 2025

Adding a filter causes the Resources menu option to not be selected:

image

This was working (e.g. you could select Graph and it would still be selected) but is broken in your PR.

var preselectedVisibleResourceTypes = VisibleTypes?.Split(',').ToHashSet();
var preselectedVisibleResourceStates = VisibleStates?.Split(',').ToHashSet();
var preselectedVisibleResourceHealthStates = VisibleHealthStates?.Split(',').ToHashSet();
var preselectedHiddenResourceTypes = HiddenTypes?.Split(' ').Select(StringUtils.Unescape).ToHashSet();
Copy link
Member

@JamesNK JamesNK Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a value (type, state, health state) contains a space?

Why did you change from comma to space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a value (type, state, health state) contains a space?

The space is serialized to a +. Then, when appending to the URL, QueryHelpers.AddQueryString will escape the + to %2B. Reverse process happens when reading from the query param.

Why did you change from comma to space?

I noticed that this was the only instance where a space was not being used to separate separate filters. In telemetry filters, space is used in the URL.

@adamint
Copy link
Member Author

adamint commented Apr 14, 2025

Adding a filter causes the Resources menu option to not be selected:

image

This was working (e.g. you could select Graph and it would still be selected) but is broken in your PR.

Fixed, thanks

@adamint adamint requested a review from JamesNK April 14, 2025 16:29
@JamesNK
Copy link
Member

JamesNK commented Apr 15, 2025

Resource filters are in the querystring when returning to the resources pages, but they aren't used:

resource-filters-not-remembered

Test all the scenarios where you could add/remove filters, and navigate to and from the resources page, e..g between table and graph, graph and another page, another page back to graph, table to another page, etc, etc

@JamesNK
Copy link
Member

JamesNK commented Apr 15, 2025

Seems this is tricky to get right. Are component tests needed here to avoid regressions?

@adamint
Copy link
Member Author

adamint commented Apr 15, 2025

Seems this is tricky to get right. Are component tests needed here to avoid regressions?

I added one to simulate the tricky case (navigation to base path, then filters are added to the url, then parameters update and filters should be applied). Do you think it's sufficient?

@adamint
Copy link
Member Author

adamint commented Apr 16, 2025

@JamesNK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AzureTools][Aspire][Question] Should filters for the Resources page be remembered?
2 participants