rusackas commented on code in PR #39460:
URL: https://github.com/apache/superset/pull/39460#discussion_r3329869968
##########
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:
Suggestion declined — `eslint-plugin-react-hooks` is listed in
devDependencies and `react-hooks/recommended` (which enables `exhaustive-deps`)
is configured in `.eslintrc.js`. The bot's rule-lookup failed; the disable
comment is valid and CI confirms it. 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:
Suggestion declined — the effect reads `markdownSource` in its conditional
guard (`component.meta.code !== markdownSource`) before deciding whether to set
state. Removing it from the dep array would freeze a stale value after the
first set, breaking the equality check. This is not an anti-pattern;
`exhaustive-deps` would object to removing it. 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]
