Fix UI issue in routing rules page - present indicative message #660

RoeyoOgen · 2025-04-02T14:05:14Z

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:

Copilot

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java

andythsu · 2025-04-09T19:20:36Z

Quickly skimmed through this. I'm fine with it

ebyhr · 2025-04-14T00:47:17Z

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java

@@ -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;


Please don't put mutable object in fields.

Not yet done. RoutingRulesConfiguration is also mutable objects.

Do you have an idea of how to inject this otherwise?

The function getRoutingRules is a route so cannot pass params...

They only thought I had was to copy the object

@andythsu any ideas maybe ?

maybe extract the fields you want and make them class variables?

in constructor:

isRulesEngineEnabled = routingRules.isRulesEngineEnabled() ruleType = routingRules.getRulesType()

notice this is omitted. I don't think we need to add this as it's redundant.

ebyhr · 2025-04-14T00:48:32Z

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java

@@ -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();


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".

reverted this, as wasn't part of scope

ebyhr · 2025-04-19T22:44:47Z

Fixed bug in UI where non indicative banner appears in routing rules page for external service

Please avoid the past tense in the commit title. https://trino.io/development/process.html#pull-request-and-commit-guidelines-

mosabua · 2025-06-18T05:48:24Z

@RoeyoOgen please update commit message to

Fix UI issue in routing rules page

and optionally have more description in the commit message body

@ebyhr @andythsu any other feedback/review comments

RoeyoOgen · 2025-06-18T12:55:49Z

@RoeyoOgen please update commit message to

Fix UI issue in routing rules page

and optionally have more description in the commit message body

@ebyhr @andythsu any other feedback/review comments

@mosabua done, lmk if anything else is needed

andythsu · 2025-06-19T05:57:08Z

webapp/src/api/base.ts

@@ -17,6 +17,14 @@ export class ClientApi {
    if (res.status === 401 || res.status === 403) {
      this.authErrorHandler()
    }
+    else if (res.status === 204) {
+      // handle the case of Response.Status.NO_CONTENT when External Routing Service is used
+      return { isExternalRouting: true };


Can we make 204 generic so it's not tied to only thrown by isExternalRouting only? Something like

const resJson = await res.json(); if (resJson.msg.type == "external_routing"){ return { isExternalRouting: true } } else{ return {} }

why?
204 is only thrown when: isRulesEngineEnabled && ruleType == RulesType.EXTERNAL under /getRoutingRules in GatewayWebAppResource so i don't seem to understand why make it genreric.

also when status === 204, the body will be empty so res.json() will be none

Uh I see. I wanted to make it generic so that we can throw 204 in other scenarios as well. But we can figure it out later if we have a use case for it

andythsu · 2025-06-19T06:04:36Z

webapp/src/components/routing-rules.tsx

-                    setFormApis(new Array(data.length).fill(null));
+                    if (data.isExternalRouting) {
+                        setIsExternalRouting(true);
+                        setRules([]);


rules is default to []. Is this still necessary?

andythsu · 2025-06-19T06:08:43Z

webapp/src/components/routing-rules.tsx

@@ -81,7 +92,31 @@ export function RoutingRules() {

    return (
            <div>


is this outer div necessary?

i tried removing it but got to many errors (and i'm not a front end guy to try and untangle this 😅)

You could replace it with <> to avoid adding an extra node to the DOM

andythsu · 2025-06-19T06:24:40Z

@RoeyoOgen please update commit message to
Fix UI issue in routing rules page
and optionally have more description in the commit message body
@ebyhr @andythsu any other feedback/review comments

@mosabua done, lmk if anything else is needed

I see you only changed the PR title but not the commit message. Could you squash your commits and update the message?

I think we can do:
commit title: Fix UI issue in routing rules page
commit body: Present indicative message when external service is used (or something along the line)

RoeyoOgen · 2025-06-19T08:03:48Z

@RoeyoOgen please update commit message to
Fix UI issue in routing rules page
and optionally have more description in the commit message body
@ebyhr @andythsu any other feedback/review comments

@mosabua done, lmk if anything else is needed

I see you only changed the PR title but not the commit message. Could you squash your commits and update the message?

I think we can do: commit title: Fix UI issue in routing rules page commit body: Present indicative message when external service is used (or something along the line)

done

andythsu · 2025-06-24T23:35:53Z

lol we can get rid of this (or something along the line) from the commit message. That was just a note for you to come up with anything you like if you don't like my suggestion 🤣

andythsu · 2025-06-24T23:36:19Z

overall I think this LGTM. @ytwp wdyt

RoeyoOgen · 2025-06-25T06:08:58Z

lol we can get rid of this (or something along the line) from the commit message. That was just a note for you to come up with anything you like if you don't like my suggestion 🤣

oopsie - fixed this :)

Present indicative message when external routing service is used or when no routing rules exist in file based routing

Chaho12

LGTM. PTAL :)

cla-bot bot added the cla-signed label Apr 2, 2025

RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch 2 times, most recently from 1be2cc6 to f1ba0b0 Compare April 6, 2025 16:42

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 requested a review from Copilot April 7, 2025 16:17

Copilot AI reviewed Apr 7, 2025

View reviewed changes

gateway-ha/src/main/java/io/trino/gateway/ha/resource/GatewayWebAppResource.java Outdated Show resolved Hide resolved

RoeyoOgen mentioned this pull request Apr 7, 2025

Add Header Modification Support to External Routing Service #654

Merged

ebyhr requested changes Apr 14, 2025

View reviewed changes

RoeyoOgen changed the title ~~Fixed bug where banner appears instead of indicative message in routing rules page~~ Fix: bug where banner appears instead of indicative message in routing rules page Apr 28, 2025

RoeyoOgen changed the title ~~Fix: bug where banner appears instead of indicative message in routing rules page~~ Fix UI issue in routing rules page - present indicative message Jun 18, 2025

RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch from f1ba0b0 to 70f60f0 Compare June 18, 2025 12:47

ebyhr assigned RoeyoOgen Jun 19, 2025

andythsu reviewed Jun 19, 2025

View reviewed changes

RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch from 826baac to 0805733 Compare June 19, 2025 08:03

RoeyoOgen requested a review from ebyhr June 24, 2025 06:32

Fix UI issue in routing rules page

7ccccb7

Present indicative message when external routing service is used or when no routing rules exist in file based routing

RoeyoOgen force-pushed the bugfix/fixing_external_service_resource_ui branch from 0805733 to 7ccccb7 Compare June 25, 2025 06:13

Chaho12 approved these changes Jul 3, 2025

View reviewed changes

RoeyoOgen requested a review from andythsu July 13, 2025 09:14

		@@ -81,7 +92,31 @@ export function RoutingRules() {

		return (
		<div>

Fix UI issue in routing rules page - present indicative message #660

Are you sure you want to change the base?

Fix UI issue in routing rules page - present indicative message #660

Uh oh!

Conversation

RoeyoOgen commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Changes

Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andythsu commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoeyoOgen Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andythsu Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Apr 19, 2025

Uh oh!

mosabua commented Jun 18, 2025

Uh oh!

RoeyoOgen commented Jun 18, 2025

Uh oh!

andythsu Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andythsu commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoeyoOgen commented Jun 19, 2025

Uh oh!

andythsu commented Jun 24, 2025

Uh oh!

andythsu commented Jun 24, 2025

Uh oh!

RoeyoOgen commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chaho12 left a comment

Choose a reason for hiding this comment

Uh oh!

RoeyoOgen commented Apr 2, 2025 •

edited

Loading

RoeyoOgen Jun 19, 2025 •

edited

Loading

andythsu Jun 19, 2025 •

edited

Loading

andythsu Jun 19, 2025 •

edited

Loading

andythsu commented Jun 19, 2025 •

edited

Loading

RoeyoOgen commented Jun 25, 2025 •

edited

Loading