refactor: Use reactDOM createRoot and single root component for all UI pages
#26426
+146
−138
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
Contributor
Reviewer's GuideThis PR refactors all UI entry points to use React 18’s createRoot API instead of ReactDOM.render, consolidates multiple mount points into single-root page components, introduces new high-level components (ClusterOverview, Worker, Stage, Plan, Query) handling layout and lazy loading (including PageTitle), updates HTML templates to match the single-root structure, and disables PropTypes lint warnings. Class diagram for new high-level page componentsclassDiagram
class ClusterOverview {
+PageTitle
+ClusterHUD (lazy)
+QueryList (lazy)
}
class Worker {
+PageTitle
+WorkerStatus (lazy)
+WorkerThreadList (lazy)
}
class Stage {
+PageTitle
+StageDetail (lazy)
}
class Plan {
+PageTitle
+LivePlan (lazy)
+getFirstParameter()
}
class Query {
+PageTitle
+QueryDetail (lazy)
}
ClusterOverview --> PageTitle
ClusterOverview --> ClusterHUD
ClusterOverview --> QueryList
Worker --> PageTitle
Worker --> WorkerStatus
Worker --> WorkerThreadList
Stage --> PageTitle
Stage --> StageDetail
Plan --> PageTitle
Plan --> LivePlan
Plan --> getFirstParameter
Query --> PageTitle
Query --> QueryDetail
Flow diagram for React root mounting refactorflowchart TD
A["HTML page with multiple mount points"] -->|Old: ReactDOM.render to #title, #cluster-hud, #query-list, etc.| B["Multiple React components mounted separately"]
B --> C["Fragmented page layout"]
D["HTML page with single #app-root"] -->|New: ReactDOM.createRoot to #app-root| E["Single high-level page component (e.g., ClusterOverview)"]
E --> F["Unified layout and lazy loading"]
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:
- It looks like the worker entrypoint (src/worker.jsx) wasn’t updated to use ReactDOM.createRoot and mount the new Worker component into the renamed
worker-rootcontainer. - The PR mixes two patterns for createRoot imports (sometimes importing createRoot directly, sometimes using ReactDOM.createRoot) – consider standardizing on one for consistency.
- PageTitle usage is duplicated in almost every new page component – you might extract a Layout or wrapper HOC to DRY up passing the same titles/urls/current props.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- It looks like the worker entrypoint (src/worker.jsx) wasn’t updated to use ReactDOM.createRoot and mount the new Worker component into the renamed `worker-root` container.
- The PR mixes two patterns for createRoot imports (sometimes importing createRoot directly, sometimes using ReactDOM.createRoot) – consider standardizing on one for consistency.
- PageTitle usage is duplicated in almost every new page component – you might extract a Layout or wrapper HOC to DRY up passing the same titles/urls/current props.
## Individual Comments
### Comment 1
<location> `presto-ui/src/components/Worker.jsx:10-17` </location>
<code_context>
+export const Worker = () => {
+ return (
+ <>
+ <div id="title">
+ <PageTitle titles={["Worker Status"]} />
+ </div>
+
+ <div id="worker-status">
+ <WorkerStatus />
+ </div>
+ <div id="worker-threads">
+ <WorkerThreadList />
+ </div>
</code_context>
<issue_to_address>
**issue:** Multiple root-level elements with IDs may cause issues with React hydration.
Consider wrapping these elements in a single root container to prevent hydration issues and avoid duplicating IDs in the static HTML.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+10
to
+17
| <div id="title"> | ||
| <PageTitle titles={["Worker Status"]} /> | ||
| </div> | ||
|
|
||
| <div id="worker-status"> | ||
| <WorkerStatus /> | ||
| </div> | ||
| <div id="worker-threads"> |
Contributor
There was a problem hiding this comment.
issue: Multiple root-level elements with IDs may cause issues with React hydration.
Consider wrapping these elements in a single root container to prevent hydration issues and avoid duplicating IDs in the static HTML.
Member
|
Hi @johallar again, thank you for the great effort of refactoring. I reviewed, checked out, ran the server with your changes, and verified all of the pages. I believe all changes are good. However, I need your help to also update the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
This makes sure we're using
createRootinstead ofrenderwhich resolves a react console warningMotivation and Context
This gets the app closer to a FE router, instead of separate html pages, saving us a bunch of round trips for deps/styles included inline.
Should also boost FE performance by allowing react to do render optimizations (eg. not completely re-render the title tabs on each nav)
Impact
This sets the FE up for performance gains in the future, nothing should change for the end user ATM
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.