Skip to content

Fixed bug where banner appears instead of indicative message in routing rules page #660

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 1 commit into
base: main
Choose a base branch
from

Conversation

RoeyoOgen
Copy link

@RoeyoOgen RoeyoOgen commented Apr 2, 2025

UI Improvements: Fixed bug where banner appears instead of indicative message in routing rules page

Changes

  • Added loading spinner while fetching routing rules
  • Fixed issue where an error banner will appear instead of an indicative message for rules managed by external routing service
  • Fixed issue where an error banner will appear instead of an indicative message for when file in 'rulesConfigPath' is empty

Impact

  • No functional changes to routing rules functionality
  • Improved user experience by eliminating incorrect empty state display

( X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Apr 2, 2025
@RoeyoOgen RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch from 4ed856b to 1be2cc6 Compare April 6, 2025 16:36
@RoeyoOgen RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch from 1be2cc6 to f1ba0b0 Compare April 6, 2025 16:42
@RoeyoOgen RoeyoOgen changed the title Fixed bug where banner appears instead of indicative message saying n… Fixed bug where banner appears instead of indicative message in routing rules page Apr 6, 2025
@RoeyoOgen RoeyoOgen requested a review from Copilot April 7, 2025 16:17
Copy link

@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 3 out of 3 changed files in this pull request and generated 1 comment.

@@ -160,7 +163,7 @@ public Response getDistribution(QueryDistributionRequest query)
Map<String, List<DistributionResponse.LineChart>> lineChartMap = lineChart.stream().collect(Collectors.groupingBy(DistributionResponse.LineChart::getName));
List<DistributionResponse.DistributionChart> distributionChart = lineChartMap.values().stream().map(d -> {
DistributionResponse.DistributionChart dc = new DistributionResponse.DistributionChart();
DistributionResponse.LineChart lc = d.get(0);
DistributionResponse.LineChart lc = d.getFirst();
Copy link
Preview

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

Ensure that 'd.getFirst()' is a valid method for the collection type of 'd'. If this collection is a standard List, use 'd.get(0)' instead.

Suggested change
DistributionResponse.LineChart lc = d.getFirst();
DistributionResponse.LineChart lc = d.get(0);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Author

Choose a reason for hiding this comment

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

This change was suggested by the Intelij IDE so changed, really have no opinion in the matter

@andythsu
Copy link
Member

andythsu commented Apr 9, 2025

Quickly skimmed through this. I'm fine with it

@@ -76,6 +77,7 @@ public class GatewayWebAppResource
// TODO Avoid putting mutable objects in fields
private final UIConfiguration uiConfiguration;
private final RoutingRulesManager routingRulesManager;
private final HaGatewayConfiguration haGatewayConfiguration;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't put mutable object in fields.

@@ -92,6 +94,7 @@ public GatewayWebAppResource(
this.resourceGroupsManager = requireNonNull(resourceGroupsManager, "resourceGroupsManager is null");
this.uiConfiguration = configuration.getUiConfiguration();
this.routingRulesManager = requireNonNull(routingRulesManager, "routingRulesManager is null");
this.haGatewayConfiguration = configuration;
Copy link
Member

Choose a reason for hiding this comment

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

Add requireNonNull.

@@ -160,7 +163,7 @@ public Response getDistribution(QueryDistributionRequest query)
Map<String, List<DistributionResponse.LineChart>> lineChartMap = lineChart.stream().collect(Collectors.groupingBy(DistributionResponse.LineChart::getName));
List<DistributionResponse.DistributionChart> distributionChart = lineChartMap.values().stream().map(d -> {
DistributionResponse.DistributionChart dc = new DistributionResponse.DistributionChart();
DistributionResponse.LineChart lc = d.get(0);
DistributionResponse.LineChart lc = d.getFirst();
Copy link
Member

Choose a reason for hiding this comment

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

Please separate a commit. This change is unrelated to "Fixed bug in UI where non indicative banner appears in routing rules page for external service".

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

Successfully merging this pull request may close these issues.

3 participants