feat: setup React hybrid widget infrastructure#16766
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a React-based TableWidget along with bundled React and Scheduler dependencies. Several critical issues were identified, including missing files (table_widget.esm.js and react-dom.js) that are required for the widget to function. Additionally, the implementation of event listeners in the table headers lacks a cleanup mechanism, which will lead to memory leaks and duplicate event handlers.
| def _esm(self): | ||
| """Load JavaScript code from external file.""" | ||
| return resources.read_text(bigframes.display, "table_widget.js") | ||
| return resources.read_text(bigframes.display, "table_widget.esm.js") |
There was a problem hiding this comment.
The pull request updates the code to load table_widget.esm.js, but this file is not included in the changes. As this is the bundled asset required for the widget to function, its absence will cause a FileNotFoundError at runtime.
| */ | ||
|
|
||
| import React, { useState, useEffect, useRef } from "../react.js"; | ||
| import ReactDOM from "../react-dom.js"; |
There was a problem hiding this comment.
The import references ../react-dom.js, but this file is missing from the pull request. While react.js and scheduler.js were added, react-dom.js is also required for the widget to render.
| header.addEventListener("mouseover", () => { | ||
| if (sortContext.findIndex((item) => item.column === columnName) === -1) { | ||
| indicatorSpan.style.visibility = "visible"; | ||
| } | ||
| }); | ||
| header.addEventListener("mouseout", () => { | ||
| if (sortContext.findIndex((item) => item.column === columnName) === -1) { | ||
| indicatorSpan.style.visibility = "hidden"; | ||
| } | ||
| }); | ||
|
|
||
| header.addEventListener("click", (event) => { | ||
| const currentSortIndex = sortContext.findIndex((item) => item.column === columnName); | ||
| let newContext = [...sortContext]; | ||
|
|
||
| if (event.shiftKey) { | ||
| if (currentSortIndex !== -1) { | ||
| if (newContext[currentSortIndex].ascending) { | ||
| newContext[currentSortIndex] = { ...newContext[currentSortIndex], ascending: false }; | ||
| } else { | ||
| newContext.splice(currentSortIndex, 1); | ||
| } | ||
| } else { | ||
| newContext.push({ column: columnName, ascending: true }); | ||
| } | ||
| } else { | ||
| if (currentSortIndex !== -1 && newContext.length === 1) { | ||
| if (newContext[currentSortIndex].ascending) { | ||
| newContext[currentSortIndex] = { ...newContext[currentSortIndex], ascending: false }; | ||
| } else { | ||
| newContext = []; | ||
| } | ||
| } else { | ||
| newContext = [{ column: columnName, ascending: true }]; | ||
| } | ||
| } | ||
|
|
||
| model.set("sort_context", newContext); | ||
| model.save_changes(); | ||
| }); |
There was a problem hiding this comment.
Event listeners (mouseover, mouseout, click) are attached to table headers inside a useEffect that re-runs whenever sortContext changes. Because there is no cleanup function to remove the previous listeners, they will accumulate on the DOM elements every time the user interacts with the table (e.g., clicking a header to sort). This leads to memory leaks and multiple executions of the click handler.
Consider using event delegation on the tableContainer for the click handler and CSS classes for hover effects to avoid attaching listeners to individual elements inside the effect.
1 participant
Set up the build pipeline and file structure to support the React Hybrid Approach for TableWidget.
esbuildvia Python pip to allow bundling without requiring a Node.js environment.bigframes/display/src/index.jsand fixed relative imports.bigframes/display/table_widget.esm.js.anywidget.pyto load the new bundled ESM file instead of the vanilla JS file.This prepares the codebase for the core rewrite of the widget functionality in React.
Fixes #<505414691> 🦕