fix: fix multi memory leak problems (#4591)

Co-authored-by: YuHong <[email protected]>

packages/drawing-ui/src/controllers/image-cropper.controller.ts

@@ -23,7 +23,7 @@ import { MessageType } from '@univerjs/design';
 import { getDrawingShapeKeyByDrawingSearch, IDrawingManagerService, SetDrawingSelectedOperation } from '@univerjs/drawing';
 import { CURSOR_TYPE, degToRad, Image, IRenderManagerService, precisionTo, Vector2 } from '@univerjs/engine-render';
 import { IMessageService } from '@univerjs/ui';
-import { filter, switchMap } from 'rxjs';
+import { of, switchMap } from 'rxjs';
 import { AutoImageCropOperation, CloseImageCropOperation, CropType, OpenImageCropOperation } from '../commands/operations/image-crop.operation';
 import { ImageCropperObject } from '../views/crop/image-cropper-object';
 
@@ -334,14 +334,14 @@ export class ImageCropperController extends Disposable {
                 imageCropperObject?.dispose();
             })
         );
-        const sheetUnit = this._univerInstanceService
+
+        const sheetUnit$ = this._univerInstanceService
             .getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET)
             .pipe(
-                filter((workbook) => Boolean(workbook)),
-                switchMap((workbook) => workbook!.activeSheet$)
+                switchMap((workbook) => workbook ? workbook.activeSheet$ : of(null))
             );
 
-        this.disposeWithMe(sheetUnit.subscribe(() => {
+        this.disposeWithMe(sheetUnit$.subscribe(() => {
             this._commandService.syncExecuteCommand(CloseImageCropOperation.id);
         }));
     }

packages/engine-render/src/render-manager/render-unit.ts

@@ -128,15 +128,6 @@ export class RenderUnit extends Disposable implements IRender {
 
         this._activated$.next(false);
         this._activated$.complete();
-
-        // Avoid memory leak. Basically it is because RenderUnit itself is leaking.
-        // We use this as a temporary solution to make CI pass.
-        // @ts-ignore
-        this._renderContext.activated$ = null;
-        // @ts-ignore
-        this._renderContext.activate = null;
-        // @ts-ignore
-        this._renderContext.deactivate = null;
     }
 
     /**

packages/sheets-drawing-ui/src/services/canvas-float-dom-manager.service.ts

@@ -28,7 +28,7 @@ import { COMMAND_LISTENER_SKELETON_CHANGE, getSheetCommandTarget, SetFrozenMutat
 import { DrawingApplyType, ISheetDrawingService, SetDrawingApplyMutation } from '@univerjs/sheets-drawing';
 import { ISheetSelectionRenderService, SetScrollOperation, SetZoomRatioOperation, SheetSkeletonManagerService } from '@univerjs/sheets-ui';
 import { CanvasFloatDomService } from '@univerjs/ui';
-import { BehaviorSubject, filter, map, Subject, switchMap, take } from 'rxjs';
+import { BehaviorSubject, filter, map, of, Subject, switchMap, take } from 'rxjs';
 import { InsertSheetDrawingCommand } from '../commands/commands/insert-sheet-drawing.command';
 
 export interface ICanvasFloatDom {
@@ -556,22 +556,27 @@ export class SheetCanvasFloatDomManagerService extends Disposable {
         // #region scroll
         this.disposeWithMe(
             this._univerInstanceService.getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET).pipe(
-                filter((workbook) => !!workbook),
-                switchMap((workbook) => workbook.activeSheet$),
-                filter((sheet) => !!sheet),
-                map((sheet) => {
-                    const render = this._renderManagerService.getRenderById(sheet.getUnitId());
-                    return render ? { render, unitId: sheet.getUnitId(), subUnitId: sheet.getSheetId() } : null;
+                switchMap((workbook) => workbook ? workbook.activeSheet$ : of(null)),
+                map((worksheet) => {
+                    if (!worksheet) return null;
+                    const unitId = worksheet.getUnitId();
+                    const render = this._renderManagerService.getRenderById(unitId);
+                    return render ? { render, unitId, subUnitId: worksheet.getSheetId() } : null;
                 }),
-                filter((render) => !!render),
                 switchMap((render) =>
-                    fromEventSubject(render.render.scene.getViewport(SHEET_VIEWPORT_KEY.VIEW_MAIN)!.onScrollAfter$)
-                        .pipe(map(() => ({ unitId: render.unitId, subUnitId: render.subUnitId })))
+                    render
+                        ? fromEventSubject(render.render.scene.getViewport(SHEET_VIEWPORT_KEY.VIEW_MAIN)!.onScrollAfter$)
+                            .pipe(map(() => ({ unitId: render.unitId, subUnitId: render.subUnitId })))
+                        : of(null)
                 )
-            ).subscribe(({ unitId, subUnitId }) => {
+            ).subscribe((value) => {
+                if (!value) return; // TODO@weird94: maybe we should throw an error here and do some cleaning work?
+
+                const { unitId, subUnitId } = value;
                 updateSheet(unitId, subUnitId);
             })
         );
+
         //#endregion
 
         // #region zoom

packages/sheets-filter/src/controllers/sheets-filter.controller.ts

@@ -199,7 +199,7 @@ export class SheetsFilterController extends Disposable {
         const filterColumn = filterModel.getAllFilterColumns();
         const effected = filterColumn.filter((column) => column[0] >= anchor);
         if (effected.length !== 0) {
-            const { newRange, oldRange } = this.moveCriteria(unitId, subUnitId, effected, count);
+            const { newRange, oldRange } = this._moveCriteria(unitId, subUnitId, effected, count);
             redos.push(...newRange.redos, ...oldRange.redos);
             undos.push(...newRange.undos, ...oldRange.undos);
         }
@@ -281,7 +281,7 @@ export class SheetsFilterController extends Disposable {
 
         let newRangeCriteria: { undos: IMutationInfo[]; redos: IMutationInfo[] } = { undos: [], redos: [] };
         if (shifted.length > 0) {
-            const { oldRange, newRange } = this.moveCriteria(unitId, subUnitId, shifted, -removeCount);
+            const { oldRange, newRange } = this._moveCriteria(unitId, subUnitId, shifted, -removeCount);
             newRangeCriteria = newRange;
             redos.push(...oldRange.redos);
             undos.unshift(...oldRange.undos);
@@ -693,7 +693,7 @@ export class SheetsFilterController extends Disposable {
         }));
     }
 
-    private moveCriteria(unitId: string, subUnitId: string, target: [number, FilterColumn][], step: number) {
+    private _moveCriteria(unitId: string, subUnitId: string, target: [number, FilterColumn][], step: number) {
         const defaultSetCriteriaMutationParams: ISetSheetsFilterCriteriaMutationParams = {
             unitId,
             subUnitId,

packages/sheets-filter/src/models/filter-model.ts

@@ -16,10 +16,11 @@
 
 import type { CellValue, ICellData, IRange, Nullable, Worksheet } from '@univerjs/core';
 import type { Observable } from 'rxjs';
+import type { IAutoFilter, ICustomFilter, ICustomFilters, IFilterColumn, IFilters } from './types';
 import { CellValueType, Disposable, extractPureTextFromCell, mergeSets, Rectangle, Tools } from '@univerjs/core';
 import { BehaviorSubject } from 'rxjs';
 import { ensureNumeric, getCustomFilterFn, isNumericFilterFn, notEquals } from './custom-filters';
-import { CustomFilterOperator, type IAutoFilter, type ICustomFilter, type ICustomFilters, type IFilterColumn, type IFilters } from './types';
+import { CustomFilterOperator } from './types';
 
 const EMPTY = () => new Set<number>();
 

packages/sheets-ui/src/controllers/auto-fill.controller.ts

@@ -40,7 +40,6 @@ import {
     IUniverInstanceService,
     ObjectMatrix,
     Rectangle,
-    toDisposable,
     Tools,
     UniverInstanceType,
 } from '@univerjs/core';
@@ -113,7 +112,7 @@ export class AutoFillController extends Disposable {
 
     private _init() {
         this._initDefaultHook();
-        this._onSelectionControlFillChanged();
+        this._initSelectionControlFillChanged();
         this._initQuitListener();
         this._initSkeletonChange();
     }
@@ -179,11 +178,10 @@ export class AutoFillController extends Disposable {
         this._autoFillService.setShowMenu(false);
     }
 
-    // eslint-disable-next-line max-lines-per-function
-    private _onSelectionControlFillChanged() {
+    private _initSelectionControlFillChanged() {
         const disposableCollection = new DisposableCollection();
-        const addListener = (disposableCollection: DisposableCollection) => {
-            // Each range change requires re-listening
+        const updateListener = () => {
+            // Each range change requires re-listening.
             disposableCollection.dispose();
 
             const currentRenderer = this._renderManagerService.getCurrentTypeOfRenderer(UniverInstanceType.UNIVER_SHEET);
@@ -192,78 +190,69 @@ export class AutoFillController extends Disposable {
             const selectionRenderService = currentRenderer.with(ISheetSelectionRenderService);
             const selectionControls = selectionRenderService.getSelectionControls();
             selectionControls.forEach((controlSelection) => {
-                disposableCollection.add(
-                    toDisposable(
-                        controlSelection.selectionFilled$.subscribe((filled) => {
-                            if (
-                                filled == null ||
+                disposableCollection.add(controlSelection.selectionFilled$.subscribe((filled) => {
+                    if (
+                        filled == null ||
                                 filled.startColumn === -1 ||
                                 filled.startRow === -1 ||
                                 filled.endColumn === -1 ||
                                 filled.endRow === -1
-                            ) {
-                                return;
-                            }
-                            const source: IRange = {
-                                startColumn: controlSelection.model.startColumn,
-                                endColumn: controlSelection.model.endColumn,
-                                startRow: controlSelection.model.startRow,
-                                endRow: controlSelection.model.endRow,
-                            };
-                            const selection: IRange = {
-                                startColumn: filled.startColumn,
-                                endColumn: filled.endColumn,
-                                startRow: filled.startRow,
-                                endRow: filled.endRow,
-                            };
-
-                            this._commandService.executeCommand(AutoFillCommand.id, { sourceRange: source, targetRange: selection });
-                        })
-                    )
-                );
+                    ) {
+                        return;
+                    }
+                    const source: IRange = {
+                        startColumn: controlSelection.model.startColumn,
+                        endColumn: controlSelection.model.endColumn,
+                        startRow: controlSelection.model.startRow,
+                        endRow: controlSelection.model.endRow,
+                    };
+                    const selection: IRange = {
+                        startColumn: filled.startColumn,
+                        endColumn: filled.endColumn,
+                        startRow: filled.startRow,
+                        endRow: filled.endRow,
+                    };
+
+                    this._commandService.executeCommand(AutoFillCommand.id, { sourceRange: source, targetRange: selection });
+                }));
 
                 // double click to fill range, range length will align to left or right column.
                 // fill results will be as same as drag operation
-                disposableCollection.add(
-                    toDisposable(
-                        controlSelection.fillControl.onDblclick$.subscribeEvent(() => {
-                            const source = {
-                                startColumn: controlSelection.model.startColumn,
-                                endColumn: controlSelection.model.endColumn,
-                                startRow: controlSelection.model.startRow,
-                                endRow: controlSelection.model.endRow,
-                            };
-                            this._handleDbClickFill(source);
-                        })
-                    )
-                );
-
-                disposableCollection.add(
-                    toDisposable(
-                        controlSelection.fillControl.onPointerDown$.subscribeEvent(() => {
-                            const visibleState = this._editorBridgeService.isVisible();
-                            if (visibleState.visible) {
-                                this._editorBridgeService.changeVisible({
-                                    visible: false,
-                                    eventType: DeviceInputEventType.PointerDown,
-                                    unitId: currentRenderer.unitId,
-                                });
-                            }
-                        })
-                    )
-                );
+                disposableCollection.add(controlSelection.fillControl.onDblclick$.subscribeEvent(() => {
+                    const source = {
+                        startColumn: controlSelection.model.startColumn,
+                        endColumn: controlSelection.model.endColumn,
+                        startRow: controlSelection.model.startRow,
+                        endRow: controlSelection.model.endRow,
+                    };
+                    this._handleDbClickFill(source);
+                }));
+
+                disposableCollection.add(controlSelection.fillControl.onPointerDown$.subscribeEvent(() => {
+                    const visibleState = this._editorBridgeService.isVisible();
+                    if (visibleState.visible) {
+                        this._editorBridgeService.changeVisible({
+                            visible: false,
+                            eventType: DeviceInputEventType.PointerDown,
+                            unitId: currentRenderer.unitId,
+                        });
+                    }
+                }));
             });
         };
 
-        addListener(disposableCollection);
+        updateListener();
 
-        this.disposeWithMe(
-            this._commandService.onCommandExecuted((command: ICommandInfo) => {
-                if (command.id === SetSelectionsOperation.id) {
-                    addListener(disposableCollection);
-                }
-            })
-        );
+        // Should subscribe current current renderer change as well.
+        // TODO@yuhongz: this seems not ideal. This should be an `IRenderModule` for running with multiple renderers?
+        this.disposeWithMe(this._commandService.onCommandExecuted((command: ICommandInfo) => {
+            if (command.id === SetSelectionsOperation.id) {
+                updateListener();
+            }
+        }));
+
+        this.disposeWithMe(this._univerInstanceService.getCurrentTypeOfUnit$(UniverInstanceType.UNIVER_SHEET)
+            .subscribe(() => updateListener()));
     }
 
     private _handleDbClickFill(source: IRange) {

packages/sheets-ui/src/controllers/clipboard/clipboard.controller.ts

@@ -913,6 +913,7 @@ export class SheetClipboardController extends RxDisposable {
         if (sheetsUIConfig?.clipboardConfig?.hidePasteOptions) {
             return;
         }
+
         this.disposeWithMe(
             this._uiPartsService.registerComponent(BuiltInUIPart.CONTENT, () => connectInjector(ClipboardPopupMenu, this._injector))
         );

packages/sheets-ui/src/controllers/format-painter/format-painter.controller.ts

@@ -30,13 +30,9 @@ import {
 } from '@univerjs/core';
 import { IRenderManagerService } from '@univerjs/engine-render';
 import { AddMergeUndoMutationFactory, AddWorksheetMergeMutation, getAddMergeMutationRangeByType, RemoveMergeUndoMutationFactory, RemoveWorksheetMergeMutation, SetRangeValuesCommand, SetRangeValuesMutation, SetRangeValuesUndoMutationFactory, SheetInterceptorService, SheetsSelectionsService } from '@univerjs/sheets';
-import {
-    ApplyFormatPainterCommand,
-    SetOnceFormatPainterCommand,
-} from '../../commands/commands/set-format-painter.command';
+
 import { checkCellContentInRanges, getClearContentMutationParamsForRanges } from '../../common/utils';
 import { FormatPainterStatus, IFormatPainterService } from '../../services/format-painter/format-painter.service';
-import { ISheetSelectionRenderService } from '../../services/selection/base-selection-render.service';
 
 export class FormatPainterController extends Disposable {
     constructor(
@@ -54,40 +50,9 @@ export class FormatPainterController extends Disposable {
     }
 
     private _initialize() {
-        this._commandExecutedListener();
         this._addDefaultHook();
     }
 
-    private _commandExecutedListener() {
-        const unitId = this._univerInstanceService.getFocusedUnit()?.getUnitId() || '';
-        const renderUnit = this._renderManagerService.getRenderById(unitId);
-        if (!renderUnit) return;
-        const selectionRenderService = renderUnit.with(ISheetSelectionRenderService);
-
-        this.disposeWithMe(
-            selectionRenderService.selectionMoveEnd$.subscribe((selections) => {
-                if (this._formatPainterService.getStatus() !== FormatPainterStatus.OFF) {
-                    const { rangeWithCoord } = selections[selections.length - 1];
-                    this._commandService.executeCommand(ApplyFormatPainterCommand.id, {
-                        unitId: this._univerInstanceService.getFocusedUnit()?.getUnitId() || '',
-                        subUnitId: (this._univerInstanceService.getFocusedUnit() as Workbook).getActiveSheet()?.getSheetId() || '',
-                        range: {
-                            startRow: rangeWithCoord.startRow,
-                            startColumn: rangeWithCoord.startColumn,
-                            endRow: rangeWithCoord.endRow,
-                            endColumn: rangeWithCoord.endColumn,
-                        },
-                    });
-
-                    // if once, turn off the format painter
-                    if (this._formatPainterService.getStatus() === FormatPainterStatus.ONCE) {
-                        this._commandService.executeCommand(SetOnceFormatPainterCommand.id);
-                    }
-                }
-            })
-        );
-    }
-
     private _addDefaultHook() {
         const defaultHook: IFormatPainterHook = {
             id: 'default-format-painter',