rusackas commented on code in PR #39460:
URL: https://github.com/apache/superset/pull/39460#discussion_r3329515409
##########
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:
Thread closed. The `react-hooks/exhaustive-deps` rule is active in this
repo: `eslint-plugin-react-hooks` is in devDependencies and
`react-hooks/recommended` is extended in `.eslintrc.js`. Bito's static analysis
toolchain failed to locate the rule definition locally; the disable comment is
valid and CI confirms it. This is a Bito loop artifact from `/resolve` not
being supported on inline comments. No further action needed.
##########
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:
Thread closed. The effect intentionally reads `markdownSource` in its
conditional check (`component.meta.code !== markdownSource`) before deciding
whether to set it. Removing it from the dependency array would freeze the
comparison and break the equality guard. The eslint-disable is intentional.
This is a Bito loop artifact from `/resolve` not being supported on inline
comments. No further action needed.
--
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]
