rusackas commented on code in PR #39460:
URL: https://github.com/apache/superset/pull/39460#discussion_r3329206126
##########
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:
Bito's tooling is failing to locate the rule definition in its static
analysis, but the rule is active: `eslint-plugin-react-hooks` is in
devDependencies and `react-hooks/recommended` is extended in `.eslintrc.js`.
The disable comment is intentional. Thread closed.
##########
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:
Agreed with bito's latest assessment — removing `markdownSource` from the
dependency array would cause stale closure issues. Rusackas's explanation
stands. Thread closed.
--
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]
