refactor(ui): Function component conversion, pt 2-1 #26467
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit f5a85d9.
Contributor
Reviewer's GuideThis PR refactors the UI layer by converting multiple React class components into functional components using hooks, modernizes helper functions into standalone arrow functions, and replaces traditional lifecycle and binding patterns with useState, useEffect, useRef, and useCallback. QueryList, QueryDetail, and QueryHeader have been updated to leverage the functional paradigm, streamlining state management, event handling, and rendering logic. Class diagram for refactored QueryList componentsclassDiagram
class QueryList {
+useState
+useRef
+useEffect
+useCallback
+sortAndLimitQueries()
+filterQueries()
+refreshLoop()
+handleSearchStringChange()
+executeSearch()
+handleMaxQueriesClick()
+handleReorderClick()
+handleSortClick()
+handleStateFilterClick()
+handleErrorTypeFilterClick()
+renderMaxQueriesListItem()
+renderReorderListItem()
+renderSortListItem()
+renderFilterButton()
+renderErrorTypeListItem()
}
class DisplayedQueriesList {
+queries
}
class QueryListItem {
+query
+renderWarning()
}
QueryList --> DisplayedQueriesList
DisplayedQueriesList --> QueryListItem
Class diagram for refactored QueryDetail componentsclassDiagram
class StageList {
+outputStage
+getStages()
}
class StageSummary {
+stage
+useState
+useRef
+useEffect
+getExpandedIcon()
+getExpandedStyle()
+toggleExpanded()
+renderHistogram()
+renderBarChart()
+renderTaskList()
+renderStageExecutionAttemptsTasks()
+handleTaskFilterClick()
+renderTaskFilterListItem()
+renderTaskFilter()
}
class RuntimeStatsList {
+stats
+useState
+getExpandedIcon()
+getExpandedStyle()
+toggleExpanded()
+renderMetricValue()
}
StageList --> StageSummary
StageSummary --> RuntimeStatsList
Class diagram for refactored QueryHeader componentclassDiagram
class QueryHeader {
+query
+renderProgressBar()
+isActive()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- QueryList bundles a lot of imperative state and ref logic—consider refactoring data fetching, filtering, and sorting into a custom hook or useReducer to replace the ad-hoc dataSet.current pattern and improve readability.
- Tooltip initialization only runs on mount, so dynamically added items may not have working Bootstrap tooltips—consider re-initializing $('[data-bs-toggle="tooltip"]').tooltip() in a useEffect after displayedQueries change.
- Handlers and inline render functions in large list components are recreated on every render; use useCallback or React.memo for stateless subcomponents to reduce unnecessary re-renders.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- QueryList bundles a lot of imperative state and ref logic—consider refactoring data fetching, filtering, and sorting into a custom hook or useReducer to replace the ad-hoc dataSet.current pattern and improve readability.
- Tooltip initialization only runs on mount, so dynamically added items may not have working Bootstrap tooltips—consider re-initializing $('[data-bs-toggle="tooltip"]').tooltip() in a useEffect after displayedQueries change.
- Handlers and inline render functions in large list components are recreated on every render; use useCallback or React.memo for stateless subcomponents to reduce unnecessary re-renders.
## Individual Comments
### Comment 1
<location> `presto-ui/src/components/QueryList.jsx:538-43` </location>
<code_context>
+ };
- renderMaxQueriesListItem(maxQueries, maxQueriesText) {
+ const handleMaxQueriesClick = (newMaxQueries) => {
+ const filteredQueries = filterQueries(dataSet.current.allQueries, dataSet.current.stateFilters, dataSet.current.errorTypeFilters, dataSet.current.searchString);
+ sortAndLimitQueries(filteredQueries, dataSet.current.currentSortType, dataSet.current.currentSortOrder, newMaxQueries);
+
+ dataSet.current.maxQueries = newMaxQueries;
+ dataSet.current.displayedQueries = filteredQueries;
+ setState(prev => ({ ...prev, maxQueries: newMaxQueries, displayedQueries: filteredQueries }));
+ };
+
+ const renderMaxQueriesListItem = (maxQueriesValue, maxQueriesText) => {
</code_context>
<issue_to_address>
**suggestion:** handleMaxQueriesClick does not prevent default on anchor click.
Since the anchor uses href="#" and a click handler, not calling event.preventDefault() may cause unwanted page jumps. Pass the event to the handler and call preventDefault to avoid this issue.
Suggested implementation:
```javascript
const handleMaxQueriesClick = (event, newMaxQueries) => {
event.preventDefault();
const filteredQueries = filterQueries(dataSet.current.allQueries, dataSet.current.stateFilters, dataSet.current.errorTypeFilters, dataSet.current.searchString);
sortAndLimitQueries(filteredQueries, dataSet.current.currentSortType, dataSet.current.currentSortOrder, newMaxQueries);
dataSet.current.maxQueries = newMaxQueries;
dataSet.current.displayedQueries = filteredQueries;
setState(prev => ({ ...prev, maxQueries: newMaxQueries, displayedQueries: filteredQueries }));
};
```
```javascript
const renderMaxQueriesListItem = (maxQueriesValue, maxQueriesText) => {
return (
<li>
<a
href="#"
className={`dropdown-item text-dark ${state.maxQueries === maxQueriesValue ? "active bg-info text-white" : "text-dark"}`}
onClick={event => handleMaxQueriesClick(event, maxQueriesValue)}
>
{maxQueriesText}
</a>
</li>
);
};
```
</issue_to_address>
### Comment 2
<location> `presto-ui/src/components/QueryDetail.jsx:510` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't reassign parameter - tasks ([`dont-reassign-parameters`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/dont-reassign-parameters))
<details><summary>Explanation</summary>Reassigning parameters can lead to unexpected behavior, especially when accessing the arguments object.
It can also cause optimization issues, especially in V8.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#functions--reassign-params)
</details>
</issue_to_address>
### Comment 3
<location> `presto-ui/src/components/QueryDetail.jsx:511` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't reassign parameter - tasks ([`dont-reassign-parameters`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/dont-reassign-parameters))
<details><summary>Explanation</summary>Reassigning parameters can lead to unexpected behavior, especially when accessing the arguments object.
It can also cause optimization issues, especially in V8.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#functions--reassign-params)
</details>
</issue_to_address>
### Comment 4
<location> `presto-ui/src/components/QueryHeader.jsx:86` </location>
<code_context>
const queryId = query.queryId;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {queryId} = query;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
cmatzenbach
changed the title
cmatzenbach
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: