bito-code-review[bot] commented on code in PR #39460:
URL: https://github.com/apache/superset/pull/39460#discussion_r3278057391
##########
superset-frontend/src/dashboard/components/SliceAdder.tsx:
##########
@@ -174,295 +164,284 @@ function getFilteredSortedSlices(
.filter(createFilter(searchTerm, KEYS_TO_FILTERS))
.sort(sortByComparator(sortBy));
}
-class SliceAdder extends Component<SliceAdderProps, SliceAdderState> {
- private slicesRequest?: AbortController | Promise<void>;
-
- static defaultProps = {
- selectedSliceIds: [],
- editMode: false,
- errorMessage: '',
- };
-
- constructor(props: SliceAdderProps) {
- super(props);
- this.state = {
- filteredSlices: [],
- searchTerm: '',
- sortBy: DEFAULT_SORT_KEY,
- selectedSliceIdsSet: new Set(props.selectedSliceIds),
- showOnlyMyCharts: getItem(
- LocalStorageKeys.DashboardEditorShowOnlyMyCharts,
- true,
- ),
- };
- this.rowRenderer = this.rowRenderer.bind(this);
- this.searchUpdated = this.searchUpdated.bind(this);
- this.handleSelect = this.handleSelect.bind(this);
- this.userIdForFetch = this.userIdForFetch.bind(this);
- this.onShowOnlyMyCharts = this.onShowOnlyMyCharts.bind(this);
- }
-
- userIdForFetch() {
- return this.state.showOnlyMyCharts ? this.props.userId : undefined;
- }
-
- componentDidMount() {
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- '',
- this.state.sortBy,
- );
- }
-
- componentDidUpdate(prevProps: SliceAdderProps) {
- const nextState: SliceAdderState = {} as SliceAdderState;
- if (this.props.lastUpdated !== prevProps.lastUpdated) {
- nextState.filteredSlices = getFilteredSortedSlices(
- this.props.slices,
- this.state.searchTerm,
- this.state.sortBy,
- this.state.showOnlyMyCharts,
- this.props.userId,
- );
- }
-
- if (prevProps.selectedSliceIds !== this.props.selectedSliceIds) {
- nextState.selectedSliceIdsSet = new Set(this.props.selectedSliceIds);
- }
-
- if (Object.keys(nextState).length) {
- this.setState(nextState);
- }
- }
- componentWillUnmount() {
- // Clears the redux store keeping only selected items
- const selectedSlices = pickBy(this.props.slices, (value: Slice) =>
- this.state.selectedSliceIdsSet.has(value.slice_id),
- );
-
- this.props.updateSlices(selectedSlices);
- if (this.slicesRequest instanceof AbortController) {
- this.slicesRequest.abort();
- }
- }
-
- handleChange = debounce(value => {
- this.searchUpdated(value);
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- value,
- this.state.sortBy,
- );
- }, 300);
-
- searchUpdated(searchTerm: string) {
- this.setState(prevState => ({
- searchTerm,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
+function SliceAdder({
+ fetchSlices,
+ updateSlices,
+ isLoading,
+ slices,
+ errorMessage = '',
+ userId,
+ selectedSliceIds = [],
+ editMode = false,
+ dashboardId,
+}: SliceAdderProps) {
+ const theme = useTheme();
+ const slicesRequestRef = useRef<AbortController | Promise<void>>();
+
+ const [searchTerm, setSearchTerm] = useState('');
+ const [sortBy, setSortBy] = useState<keyof Slice>(DEFAULT_SORT_KEY);
+ const [selectedSliceIdsSet, setSelectedSliceIdsSet] = useState(
+ () => new Set(selectedSliceIds),
+ );
+
+ // Refs to track latest values for cleanup effect
+ const latestSlicesRef = useRef(slices);
+ const latestSelectedSliceIdsSetRef = useRef(selectedSliceIdsSet);
+ const [showOnlyMyCharts, setShowOnlyMyCharts] = useState(() =>
+ getItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts, true),
+ );
+
+ // Keep refs updated with latest values
+ useEffect(() => {
+ latestSlicesRef.current = slices;
+ }, [slices]);
+
+ useEffect(() => {
+ latestSelectedSliceIdsSetRef.current = selectedSliceIdsSet;
+ }, [selectedSliceIdsSet]);
+
+ const filteredSlices = useMemo(
+ () =>
+ getFilteredSortedSlices(
+ slices,
searchTerm,
- prevState.sortBy,
- prevState.showOnlyMyCharts,
- this.props.userId,
- ),
- }));
- }
-
- handleSelect(sortBy: keyof Slice) {
- this.setState(prevState => ({
- sortBy,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
- prevState.searchTerm,
sortBy,
- prevState.showOnlyMyCharts,
- this.props.userId,
- ),
- }));
- this.slicesRequest = this.props.fetchSlices(
- this.userIdForFetch(),
- this.state.searchTerm,
- sortBy,
- );
- }
-
- rowRenderer({ index, style }: { index: number; style: React.CSSProperties })
{
- const { filteredSlices, selectedSliceIdsSet } = this.state;
- const cellData = filteredSlices[index];
-
- const isSelected = selectedSliceIdsSet.has(cellData.slice_id);
- const type = CHART_TYPE;
- const id = NEW_CHART_ID;
-
- const meta = {
- chartId: cellData.slice_id,
- sliceName: cellData.slice_name,
- };
- return (
- <DragDroppable
- key={cellData.slice_id}
- component={{ type, id, meta }}
- parentComponent={{
- id: NEW_COMPONENTS_SOURCE_ID,
- type: NEW_COMPONENT_SOURCE_TYPE,
- }}
- index={index}
- depth={0}
- disableDragDrop={isSelected}
- editMode={this.props.editMode}
- // we must use a custom drag preview within the List because
- // it does not seem to work within a fixed-position container
- useEmptyDragPreview
- // List library expect style props here
- // actual style should be applied to nested AddSliceCard component
- style={{}}
- >
- {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => (
- <AddSliceCard
- innerRef={dragSourceRef}
- style={style}
- sliceName={cellData.slice_name}
- lastModified={cellData.changed_on_humanized}
- visType={cellData.viz_type}
- datasourceUrl={cellData.datasource_url}
- datasourceName={cellData.datasource_name}
- thumbnailUrl={cellData.thumbnail_url}
- isSelected={isSelected}
- />
- )}
- </DragDroppable>
- );
- }
-
- onShowOnlyMyCharts = (showOnlyMyCharts: boolean) => {
- if (!showOnlyMyCharts) {
- this.slicesRequest = this.props.fetchSlices(
- undefined,
- this.state.searchTerm,
- this.state.sortBy,
- );
- }
- this.setState(prevState => ({
- showOnlyMyCharts,
- filteredSlices: getFilteredSortedSlices(
- this.props.slices,
- prevState.searchTerm,
- prevState.sortBy,
showOnlyMyCharts,
- this.props.userId,
+ userId,
),
- }));
- setItem(LocalStorageKeys.DashboardEditorShowOnlyMyCharts,
showOnlyMyCharts);
- };
+ [slices, searchTerm, sortBy, showOnlyMyCharts, userId],
+ );
+
+ const userIdForFetch = useCallback(
+ () => (showOnlyMyCharts ? userId : undefined),
+ [showOnlyMyCharts, userId],
+ );
+
+ // componentDidMount
+ useEffect(() => {
+ slicesRequestRef.current = fetchSlices(userIdForFetch(), '', sortBy);
+ // Only run on mount
+ // eslint-disable-next-line react-hooks/exhaustive-deps
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>ESLint rule definition not found for
exhaustive-deps</b></div>
<div id="fix">
The ESLint rule `react-hooks/exhaustive-deps` is referenced in a disable
comment on line 225, but the rule definition is not found. Ensure the
`eslint-plugin-react-hooks` package is properly installed and configured in
your ESLint setup.
</div>
</div>
<small><i>Code Review Run #dabf1d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx:
##########
@@ -140,193 +132,200 @@ interface DragChildProps {
dragSourceRef: React.RefCallback<HTMLElement>;
}
-class Markdown extends PureComponent<MarkdownProps, MarkdownState> {
- renderStartTime: number;
-
- constructor(props: MarkdownProps) {
- super(props);
- this.state = {
- isFocused: false,
- markdownSource: props.component.meta.code as string,
- editor: null,
- editorMode: 'preview',
- undoLength: props.undoLength,
- redoLength: props.redoLength,
- };
- this.renderStartTime = Logger.getTimestamp();
-
- this.handleChangeFocus = this.handleChangeFocus.bind(this);
- this.handleChangeEditorMode = this.handleChangeEditorMode.bind(this);
- this.handleMarkdownChange = this.handleMarkdownChange.bind(this);
- this.handleDeleteComponent = this.handleDeleteComponent.bind(this);
- this.handleResizeStart = this.handleResizeStart.bind(this);
- this.setEditor = this.setEditor.bind(this);
- this.shouldFocusMarkdown = this.shouldFocusMarkdown.bind(this);
- }
-
- componentDidMount(): void {
- this.props.logEvent(LOG_ACTIONS_RENDER_CHART, {
- viz_type: 'markdown',
- start_offset: this.renderStartTime,
- ts: new Date().getTime(),
- duration: Logger.getTimestamp() - this.renderStartTime,
- });
- }
-
- static getDerivedStateFromProps(
- nextProps: MarkdownProps,
- state: MarkdownState,
- ): MarkdownState | null {
- const { hasError, editorMode, markdownSource, undoLength, redoLength } =
- state;
- const {
- component: nextComponent,
- undoLength: nextUndoLength,
- redoLength: nextRedoLength,
- } = nextProps;
- // user click undo or redo ?
- if (nextUndoLength !== undoLength || nextRedoLength !== redoLength) {
- return {
- ...state,
- undoLength: nextUndoLength,
- redoLength: nextRedoLength,
- markdownSource: nextComponent.meta.code as string,
- hasError: false,
- };
- }
+function Markdown({
+ id,
+ parentId,
+ component,
+ parentComponent,
+ index,
+ depth,
+ editMode,
+ availableColumnCount,
+ columnWidth,
+ onResizeStart,
+ onResize,
+ onResizeStop,
+ deleteComponent,
+ handleComponentDrop,
+ updateComponents,
+ logEvent,
+ addDangerToast,
+ undoLength,
+ redoLength,
+ htmlSanitization,
+ htmlSchemaOverrides,
+}: MarkdownProps) {
+ const [isFocused, setIsFocused] = useState(false);
+ const [markdownSource, setMarkdownSource] = useState<string>(
+ component.meta.code as string,
+ );
+ const [editor, setEditorState] = useState<EditorInstance | null>(null);
+ const [editorMode, setEditorMode] = useState<'preview' | 'edit'>('preview');
+ const [hasError, setHasError] = useState(false);
+
+ const renderStartTimeRef = useRef(Logger.getTimestamp());
+ const prevUndoLengthRef = useRef(undoLength);
+ const prevRedoLengthRef = useRef(redoLength);
+ const prevComponentWidthRef = useRef(component.meta.width);
+ const prevColumnWidthRef = useRef(columnWidth);
+
+ // getDerivedStateFromProps equivalent: handle undo/redo and external code
changes
+ useEffect(() => {
+ // user click undo or redo?
if (
+ undoLength !== prevUndoLengthRef.current ||
+ redoLength !== prevRedoLengthRef.current
+ ) {
+ setMarkdownSource(component.meta.code as string);
+ setHasError(false);
+ prevUndoLengthRef.current = undoLength;
+ prevRedoLengthRef.current = redoLength;
+ } else if (
!hasError &&
editorMode === 'preview' &&
- nextComponent.meta.code !== markdownSource
+ component.meta.code !== markdownSource
) {
- return {
- ...state,
- markdownSource: nextComponent.meta.code as string,
- };
+ setMarkdownSource(component.meta.code as string);
}
+ }, [
+ undoLength,
+ redoLength,
+ component.meta.code,
+ hasError,
+ editorMode,
+ markdownSource,
+ ]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>useEffect dependency issue</b></div>
<div id="fix">
useEffect dependency array includes `markdownSource` which is also set
inside the effect (line 188). While guarded by a condition, this causes
unnecessary re-runs and is a React anti-pattern. Remove `markdownSource` from
dependencies.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx
+++
superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.tsx
@@ -187,8 +187,6 @@
}, [
undoLength,
redoLength,
component.meta.code,
hasError,
editorMode,
- markdownSource,
]);
```
</div>
</details>
</div>
<small><i>Code Review Run #dabf1d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
