Skip to content

Commit e97b004

Browse files
cmatzenbachcmatzenbach
authored
refactor(ui): Convert components from class to function components, pt 1 (#25759)
## Description Second batch of the component conversion started in #25666 (please see PR for background details). This focuses on the components in the `components/` folder; due to the amount of components there and large diffs, we will break up into a few separate PRs. In addition, I also fixed what I assume is a bug related to displaying the `LivePlan` component - previously, the function that sets up the D3 graph (`updateD3Graph`), was only called from `componentDidUpdate`, and had a check on it: ``` componentDidUpdate(prevProps: LivePlanProps, prevState: LivePlanState) { if (this.state.query !== prevState.query) { this.updateD3Graph(); } //$FlowFixMe $('[data-bs-toggle="tooltip"]')?.tooltip?.() } ``` However, after the first render, `this.state.query` and `prevState.query` are both identical, causing `updateD3Graph` to only run once. The problem is, that function has a condition where it sets up the `svg` and then shorts out: ``` updateD3Graph() { if (!this.state.svg) { this.setState({ svg: initializeSvg("#plan-canvas"), }); return; } ... ``` Thus, it will only run once, only setting up `this.state.svg`, and then shorting out, preventing the graph from being displayed. However, this might be situational, as I have only tested this on finished queries - maybe this is only supposed to show for running queries? I'm not sure, but I have tested on master, and if you go into Query Details -> Live Plan for a finished query, you will not see the chart. My refactoring has resolved this, but please let me know if this is intended functionality. ## Motivation and Context Resolves item 12 in [#21062](#21062); function components are the new de-facto standard in react, and benefit from ongoing optimizations in current and future react versions. ## Impact UI ## Test Plan Tested the UI interface extensively, checking all refactored components and their interactions. Components/interactions tested: - ClusterHUD - including sparkline charts - LivePlan - including bug fix for finished queries - PageTitle - SQLClientView, including running queries and other interactions - StaticQueryHeader - WorkerStatus, including sparkline charts - WorkerThreadList ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. ## Release Notes ``` == NO RELEASE NOTE == ``` ## Summary by Sourcery Convert multiple React class components to function components using hooks, consolidate helper logic into standalone functions, and fix LivePlan D3 graph rendering for completed queries. Bug Fixes: - Fix LivePlan D3 graph initialization so the chart displays correctly for finished queries. Enhancements: - Refactor WorkerStatus, WorkerThreadList, LivePlan (including StageStatistics and PlanNode), PageTitle, ClusterHUD, SQLInput, StaticQueryHeader, and SQLClientView into function components with useState, useEffect, useRef, and useCallback hooks. - Convert static methods and class utilities (e.g., status/thread queries, thread processing, stage flattening) into standalone helper functions for improved code clarity and modularity.
1 parent a651955 commit e97b004

File tree

8 files changed

+1028
-1049
lines changed

8 files changed

+1028
-1049
lines changed

‎presto-ui/src/components/ClusterHUD.jsx‎

Lines changed: 104 additions & 103 deletions
Large diffs are not rendered by default.

‎presto-ui/src/components/LivePlan.jsx‎

Lines changed: 249 additions & 237 deletions
Large diffs are not rendered by default.

‎presto-ui/src/components/PageTitle.jsx‎

Lines changed: 120 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* limitations under the License.
1313
*/
1414
//@flow
15-
import React from "react";
15+
import React, { useState, useEffect, useRef } from "react";
1616

1717
type Props = {
1818
titles: string[],
@@ -30,7 +30,7 @@ type State = {
3030
errorText: ?string,
3131
}
3232

33-
function ClusterResourceGroupNavBar({titles, urls, current = 0} : Props) {
33+
const ClusterResourceGroupNavBar = ({titles, urls, current = 0} : Props) => {
3434
const classNames = ['navbar-brand inactive', 'navbar-brand'];
3535
const navBarItems = titles.map( (title, index) => {
3636
const classNameIdx = (current === index || !urls?.length) ? 0 : 1;
@@ -42,157 +42,162 @@ function ClusterResourceGroupNavBar({titles, urls, current = 0} : Props) {
4242
</td>
4343
);
4444
});
45+
4546
return (
4647
<>{navBarItems}</>
4748
);
48-
}
49+
};
4950

50-
function isOffline() {
51+
const isOffline = () => {
5152
return window.location.protocol === 'file:';
52-
}
53+
};
5354

54-
export class PageTitle extends React.Component<Props, State> {
55-
timeoutId: TimeoutID;
55+
export const PageTitle = (props: Props): React.Node => {
56+
const [state, setState] = useState<State>({
57+
noConnection: false,
58+
lightShown: false,
59+
info: null,
60+
lastSuccess: Date.now(),
61+
modalShown: false,
62+
errorText: null,
63+
});
5664

57-
constructor(props: Props) {
58-
super(props);
59-
this.state = {
60-
noConnection: false,
61-
lightShown: false,
62-
info: null,
63-
lastSuccess: Date.now(),
64-
modalShown: false,
65-
errorText: null,
66-
};
67-
}
65+
const timeoutId = useRef<TimeoutID | null>(null);
6866

69-
refreshLoop: () => void = () => {
70-
clearTimeout(this.timeoutId);
67+
const refreshLoop = () => {
68+
clearTimeout(timeoutId.current);
7169
fetch("/v1/info")
7270
.then(response => response.json())
7371
.then(info => {
74-
this.setState({
72+
setState(prevState => ({
73+
...prevState,
7574
info: info,
7675
noConnection: false,
7776
lastSuccess: Date.now(),
7877
modalShown: false,
79-
});
78+
}));
8079
//$FlowFixMe$ Bootstrap 5 plugin
8180
$('#no-connection-modal').hide();
82-
this.resetTimer();
81+
timeoutId.current = setTimeout(refreshLoop, 1000);
8382
})
8483
.catch(error => {
85-
this.setState({
86-
noConnection: true,
87-
lightShown: !this.state.lightShown,
88-
errorText: error
89-
});
90-
this.resetTimer();
84+
setState(prevState => {
85+
const noConnection = true;
86+
const lightShown = !prevState.lightShown;
87+
const errorText = error;
88+
const shouldShowModal = !prevState.modalShown && (error || (Date.now() - prevState.lastSuccess) > 30 * 1000);
89+
90+
if (shouldShowModal) {
91+
//$FlowFixMe$ Bootstrap 5 plugin
92+
$('#no-connection-modal').modal('show');
93+
}
9194

92-
if (!this.state.modalShown && (error || (Date.now() - this.state.lastSuccess) > 30 * 1000)) {
93-
//$FlowFixMe$ Bootstrap 5 plugin
94-
$('#no-connection-modal').modal();
95-
this.setState({modalShown: true});
96-
}
97-
});
98-
}
99-
100-
resetTimer() {
101-
clearTimeout(this.timeoutId);
102-
this.timeoutId = setTimeout(this.refreshLoop.bind(this), 1000);
103-
}
95+
timeoutId.current = setTimeout(refreshLoop, 1000);
96+
97+
return {
98+
...prevState,
99+
noConnection,
100+
lightShown,
101+
errorText,
102+
modalShown: shouldShowModal || prevState.modalShown
103+
};
104+
});
105+
});
106+
};
104107

105-
componentDidMount() {
106-
if ( isOffline() ) {
107-
this.setState({
108+
useEffect(() => {
109+
if (isOffline()) {
110+
setState(prevState => ({
111+
...prevState,
108112
noConnection: true,
109113
lightShown: true,
110-
});
111-
}
112-
else {
113-
this.refreshLoop();
114+
}));
115+
} else {
116+
refreshLoop();
114117
}
115-
}
116118

117-
renderStatusLight(): any {
118-
if (this.state.noConnection) {
119-
if (this.state.lightShown) {
119+
return () => {
120+
clearTimeout(timeoutId.current);
121+
};
122+
}, []);
123+
124+
const renderStatusLight = () => {
125+
if (state.noConnection) {
126+
if (state.lightShown) {
120127
return <span className="status-light status-light-red" id="status-indicator"/>;
128+
} else {
129+
return <span className="status-light" id="status-indicator"/>;
121130
}
122-
else {
123-
return <span className="status-light" id="status-indicator"/>
124-
}
131+
} else {
132+
return <span className="status-light status-light-green" id="status-indicator"/>;
125133
}
126-
return <span className="status-light status-light-green" id="status-indicator"/>;
127-
}
134+
};
128135

129-
render(): any {
130-
const info = this.state.info;
131-
if (!isOffline() && !info) {
132-
return null;
133-
}
136+
const { info } = state;
137+
if (!isOffline() && !info) {
138+
return null;
139+
}
134140

135-
return (
136-
<div>
137-
<nav className="navbar navbar-expand-lg navbar-dark bg-dark">
138-
<div className="container-fluid">
139-
<div className="navbar-header">
140-
<table>
141-
<tbody>
141+
return (
142+
<div>
143+
<nav className="navbar navbar-expand-lg navbar-dark bg-dark">
144+
<div className="container-fluid">
145+
<div className="navbar-header">
146+
<table>
147+
<tbody>
142148
<tr>
143149
<td>
144-
<a href="/ui/"><img src={`${this.props.path ? this.props.path : '.'}/assets/logo.png`}/></a>
150+
<a href="/ui/"><img src={`${props.path || '.'}/assets/logo.png`}/></a>
145151
</td>
146-
<ClusterResourceGroupNavBar titles={this.props.titles} urls={this.props.urls} current={this.props.current} />
152+
<ClusterResourceGroupNavBar titles={props.titles} urls={props.urls} current={props.current} />
147153
</tr>
148-
</tbody>
149-
</table>
150-
</div>
151-
<button className="navbar-toggler" type="button" data-bs-toggle="collapse" data-bs-target="#navbar" aria-controls="navbar" aria-expanded="false" aria-label="Toggle navigation">
152-
<span className="navbar-toggler-icon"></span>
153-
</button>
154-
<div id="navbar" className="navbar-collapse collapse">
155-
<ul className="nav navbar-nav navbar-right ms-auto">
156-
<li>
157-
<span className="navbar-cluster-info">
158-
<span className="uppercase">Version</span><br/>
159-
<span className="text" id="version-number">{isOffline() ? 'N/A' : info?.nodeVersion?.version}</span>
160-
</span>
161-
</li>
162-
<li>
163-
<span className="navbar-cluster-info">
164-
<span className="uppercase">Environment</span><br/>
165-
<span className="text" id="environment">{isOffline() ? 'N/A' : info?.environment}</span>
166-
</span>
167-
</li>
168-
<li>
169-
<span className="navbar-cluster-info">
170-
<span className="uppercase">Uptime</span><br/>
171-
<span data-bs-toggle="tooltip" data-bs-placement="bottom" title="Connection status">
172-
{this.renderStatusLight()}
173-
</span>
174-
&nbsp;
175-
<span className="text" id="uptime">{isOffline() ? 'Offline' : info?.uptime}</span>
154+
</tbody>
155+
</table>
156+
</div>
157+
<button className="navbar-toggler" type="button" data-bs-toggle="collapse" data-bs-target="#navbar" aria-controls="navbar" aria-expanded="false" aria-label="Toggle navigation">
158+
<span className="navbar-toggler-icon"></span>
159+
</button>
160+
<div id="navbar" className="navbar-collapse collapse">
161+
<ul className="nav navbar-nav navbar-right ms-auto">
162+
<li>
163+
<span className="navbar-cluster-info">
164+
<span className="uppercase">Version</span><br/>
165+
<span className="text" id="version-number">{isOffline() ? 'N/A' : info?.nodeVersion?.version}</span>
166+
</span>
167+
</li>
168+
<li>
169+
<span className="navbar-cluster-info">
170+
<span className="uppercase">Environment</span><br/>
171+
<span className="text" id="environment">{isOffline() ? 'N/A' : info?.environment}</span>
172+
</span>
173+
</li>
174+
<li>
175+
<span className="navbar-cluster-info">
176+
<span className="uppercase">Uptime</span><br/>
177+
<span data-bs-toggle="tooltip" data-bs-placement="bottom" title="Connection status">
178+
{renderStatusLight()}
176179
</span>
177-
</li>
178-
</ul>
179-
</div>
180+
&nbsp;
181+
<span className="text" id="uptime">{isOffline() ? 'Offline' : info?.uptime}</span>
182+
</span>
183+
</li>
184+
</ul>
180185
</div>
181-
</nav>
182-
<div id="no-connection-modal" className="modal" tabIndex="-1" role="dialog">
183-
<div className="modal-dialog modal-sm" role="document">
184-
<div className="modal-content">
185-
<div className="row error-message">
186-
<div className="col-12">
187-
<br />
188-
<h4>Unable to connect to server</h4>
189-
<p>{this.state.errorText ? "Error: " + this.state.errorText : null}</p>
190-
</div>
186+
</div>
187+
</nav>
188+
<div id="no-connection-modal" className="modal" tabIndex="-1" role="dialog">
189+
<div className="modal-dialog modal-sm" role="document">
190+
<div className="modal-content">
191+
<div className="row error-message">
192+
<div className="col-12">
193+
<br />
194+
<h4>Unable to connect to server</h4>
195+
<p>{state.errorText ? "Error: " + state.errorText : null}</p>
191196
</div>
192197
</div>
193198
</div>
194199
</div>
195200
</div>
196-
);
197-
}
198-
}
201+
</div>
202+
);
203+
};

‎presto-ui/src/components/SQLClient.jsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ type SessionValues = {
3030
[key: string]: string;
3131
};
3232

33-
export default function SQLClientView() {
33+
const SQLClientView = () => {
3434
const [values, setValues] = React.useState({sql: '', running: false, results: undefined, view: 'SQL'});
3535
const sessions: SessionValues = React.useRef({});
3636
const views = [{name: 'SQL', label: 'SQL'}, {name: 'Session', label: 'Session Properties'}];
@@ -84,7 +84,7 @@ export default function SQLClientView() {
8484
<div className="col-12">
8585
<nav className="nav nav-tabs">
8686
{views.map((view, idx) => (
87-
<a className={clsx('nav-link', values.view === view.name && 'active')} href="#" onClick={() => switchView(view)}>{view.label}</a>
87+
<a key={view.name} className={clsx('nav-link', values.view === view.name && 'active')} href="#" onClick={() => switchView(view)}>{view.label}</a>
8888
))}
8989
</nav>
9090
</div>
@@ -111,6 +111,7 @@ export default function SQLClientView() {
111111
</div>
112112
</>
113113
);
114-
115114
}
116115

116+
export default SQLClientView;
117+

‎presto-ui/src/components/SQLInput.jsx‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* limitations under the License.
1313
*/
1414

15-
import React from 'react';
15+
import React, { useState, useEffect } from 'react';
1616
import antlr4 from 'antlr4';
1717
import SqlBaseLexer from '../sql-parser/SqlBaseLexer.js';
1818
import SqlBaseParser from '../sql-parser/SqlBaseParser.js';
@@ -27,7 +27,7 @@ import { clsx } from 'clsx';
2727
import PrestoClient from "@prestodb/presto-js-client";
2828

2929
// Create PrestoClient which always uses the current domain
30-
export function createClient(catalog: string, schema: string, sessions: string): PrestoClient {
30+
export const createClient = (catalog: string, schema: string, sessions: string): PrestoClient => {
3131
const opt: PrestoClientConfig = {
3232
host: `${window.location.protocol}//${window.location.hostname}`,
3333
port: (window.location.port || (window.location.protocol === 'https:' ? '443' : '80')),
@@ -92,7 +92,7 @@ class SyntaxError extends antlr4.error.ErrorListener {
9292
}
9393

9494
// Dropdown for Catalog and Schema
95-
function SQLDropDown({ text, values = [], onSelect, selected }) {
95+
const SQLDropDown = ({ text, values = [], onSelect, selected }) => {
9696

9797
function selectItem(item) {
9898
if (item !== selected) {
@@ -114,7 +114,7 @@ function SQLDropDown({ text, values = [], onSelect, selected }) {
114114
);
115115
}
116116

117-
function sqlCleaning(sql, errorHandler) {
117+
const sqlCleaning = (sql, errorHandler) => {
118118
const lastSemicolon = /(\s*;\s*)$/m;
119119
const limitRE = /limit\s+(\d+|all)$/im;
120120
const fetchFirstRE = /fetch\s+first\s+(\d+)\s+rows\s+only$/im;
@@ -150,12 +150,12 @@ function sqlCleaning(sql, errorHandler) {
150150
}
151151

152152
// SQL Input, including sql text, catalog(optional), and schema(optional)
153-
export function SQLInput({ handleSQL, show, enabled, initialSQL, errorHandler }) {
154-
const [sql, setSql] = React.useState(initialSQL);
155-
const [catalog, setCatalog] = React.useState(undefined);
156-
const [schema, setSchema] = React.useState(undefined);
157-
const [catalogs, setCatalogs] = React.useState([]);
158-
const [schemas, setSchemas] = React.useState([]);
153+
export const SQLInput = ({ handleSQL, show, enabled, initialSQL, errorHandler }) => {
154+
const [sql, setSql] = useState(initialSQL);
155+
const [catalog, setCatalog] = useState(undefined);
156+
const [schema, setSchema] = useState(undefined);
157+
const [catalogs, setCatalogs] = useState([]);
158+
const [schemas, setSchemas] = useState([]);
159159

160160
const checkValue = () => {
161161
if (sql.length > 0) {
@@ -186,7 +186,7 @@ export function SQLInput({ handleSQL, show, enabled, initialSQL, errorHandler })
186186
setSchema(schema);
187187
}
188188

189-
React.useEffect(() => {
189+
useEffect(() => {
190190
//fetch catalogs:
191191
async function getCatalogs() {
192192
const client = createClient();

0 commit comments

Comments
 (0)