Skip to content
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

Refactor console logs page and friends #5703

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Sep 13, 2024

I really looked into what the console page is doing and why issues keep coming up. Problems I see:

  • The page is reinitializing the query on every parameter update. It should only care when the selected resource changes.
  • There are multiple cancellation tokens at work (one on page, on in log viewer control).
  • The log viewer control seems unnessarily complex. Can be much simpler if the page pushes data into it.
  • No way to cancel a resources logs subscription and then wait for it to clean up before starting new subscription. Created race conditions in state

Changes in PR:

  • Only update console query if the selected resource changes. Remove hacks to ignore resizing and focus on when the query actually needs to change.
  • Fix opening mobile view of the select resource dialog. I ran into this while testing and I think this is an unnoticed regression on main. I don't know when it started. Blazor was removing the selected value because it didn't match the any value in the resource list. The resource subscription event on this page changes the resource list. Improved by adding equality interfaces to bound values.
  • Removed the JS display update for the select list. It doesn't appear to me to be needed anymore. I see items updating as resources are started and stop. Maybe the fix above sorted the issue out. @adamint Please double check that all scenarios are still working after it is removed.
  • Simplify log viewer to accept logs given to it rather than reading from the source itself.
  • Adds ConsoleLogs page test

Fixes #5691

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?
    • Yes
      • Link to aspire-docs issue:
    • No

@JamesNK JamesNK changed the title More console logging page fixes Refactor console logs page and friends Sep 14, 2024
Copy link
Member

@adamint adamint left a comment

Choose a reason for hiding this comment

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

all scenarios seem ok

@adamint adamint merged commit 6b4bfbb into main Sep 16, 2024
11 checks passed
@adamint adamint deleted the jamesnk/consolelogs-page branch September 16, 2024 16:09
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants