ci: disposing unit should not emit error (#2529)

* ci: should throw no error on disposing unit

* ci: trigger error

* chore: fix code style

* fix: change logic of permission menu

* fix: fix init value

* chore: change export

* fix: bugfix

* fix: fix disposing sequence

* chore: remove redundant code

* fix: clear sheet permission when dispose unit

* fix: fix time sequence error

* fix: clear permission point when dispose unit

* fix: fix test

---------

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

e2e/disposing/disposing.spec.ts

@@ -43,10 +43,30 @@ test('no error on constructing and disposing', async ({ page }) => {
  await page.waitForTimeout(2000);
 
  await page.evaluate(() => window.E2EControllerAPI.loadDefaultSheet());
- await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
+ await page.waitForTimeout(2000);
 
  await page.evaluate(() => window.E2EControllerAPI.disposeUniver());
- await page.waitForTimeout(2000); // wait for long enough to let the GC do its job
+ await page.waitForTimeout(2000);
+
+ expect(errored).toBeFalsy();
+});
+
+test('no error when dispose a unit', async ({ page }) => {
+ let errored = false;
+
+ page.on('pageerror', (error) => {
+ console.error('Page error:', error);
+ errored = true;
+ });
+
+ await page.goto('http:https://localhost:3000/sheets/');
+ await page.waitForTimeout(2000);
+
+ await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(1));
+ await page.waitForTimeout(2000);
+
+ await page.evaluate(() => window.E2EControllerAPI.loadAndRelease(2));
+ await page.waitForTimeout(2000);
 
  expect(errored).toBeFalsy();
 });

e2e/memory/memory.spec.ts

@@ -23,6 +23,7 @@ import { expect, test } from '@playwright/test';
 // examples/src/plugins/debugger/controllers/e2e/e2e-memory.controller.ts
 export interface IE2EControllerAPI {
  loadAndRelease(id: number): Promise<void>;
+ loadDefaultSheet(): Promise<void>;
  disposeUniver(): Promise<void>;
 }
 
@@ -33,6 +34,8 @@ declare global {
  }
 }
 
+const MAX_MEMORY_OVERFLOW = 2_500_000;
+
 test('memory', async ({ page }) => {
  await page.goto('http:https://localhost:3000/sheets/');
  await page.waitForTimeout(2000);
@@ -49,9 +52,8 @@ test('memory', async ({ page }) => {
  const memoryAfterSecondLoad = (await getMetrics(page)).JSHeapUsedSize;
  console.log('Memory after second load:', memoryAfterSecondLoad);
 
- // max overflow for 3MB
  const notLeaking = (memoryAfterSecondLoad <= memoryAfterFirstLoad)
- || (memoryAfterSecondLoad - memoryAfterFirstLoad <= 2_500_000);
+ || (memoryAfterSecondLoad - memoryAfterFirstLoad <= MAX_MEMORY_OVERFLOW);
  expect(notLeaking).toBeTruthy();
 });
 

packages/core/src/services/instance/instance.service.ts

@@ -119,21 +119,28 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
  };
  }
 
- private readonly _currentUnits$ = new BehaviorSubject<{ [type: UnitType]: Nullable<UnitModel> }>({});
+ private _currentUnits = new Map<UnitType, Nullable<UnitModel>>();
+ private readonly _currentUnits$ = new BehaviorSubject<Map<UnitType, Nullable<UnitModel>>>(this._currentUnits);
  readonly currentUnits$ = this._currentUnits$.asObservable();
  getCurrentTypeOfUnit$<T>(type: number): Observable<Nullable<T>> {
- return this.currentUnits$.pipe(map((units) => units[type] ?? null), distinctUntilChanged()) as Observable<Nullable<T>>;
+ return this.currentUnits$.pipe(map((units) => units.get(type) ?? null), distinctUntilChanged()) as Observable<Nullable<T>>;
  }
 
  getCurrentUnitForType<T>(type: UnitType): Nullable<T> {
- return this._currentUnits$.getValue()[type] as Nullable<T>;
+ return this._currentUnits.get(type) as Nullable<T>;
  }
 
  setCurrentUnitForType(unitId: string): void {
  const result = this._getUnitById(unitId);
  if (!result) throw new Error(`[UniverInstanceService]: no document with unitId ${unitId}!`);
 
- this._currentUnits$.next({ ...this._currentUnits$.getValue(), [result[1]]: result[0] });
+ this._currentUnits.set(result[1], result[0]);
+ this._currentUnits$.next(this._currentUnits);
+ }
+
+ private _removeCurrentUnitForType(type: UnitType): void {
+ this._currentUnits.set(type, null);
+ this._currentUnits$.next(this._currentUnits);
  }
 
  private readonly _unitAdded$ = new Subject<UnitModel>();
@@ -142,21 +149,28 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
  return this._unitAdded$.pipe(filter((unit) => unit.type === type)) as Observable<T>;
  }
 
+ /**
+ * Add a unit into Univer.
+ *
+ * @ignore
+ *
+ * @param unit The unit to be added.
+ */
  __addUnit(unit: UnitModel): void {
  const type = unit.type;
 
  if (!this._unitsByType.has(type)) {
  this._unitsByType.set(type, []);
  }
-
  this._unitsByType.get(type)!.push(unit);
 
- this._currentUnits$.next({ ...this._currentUnits$.getValue(), [type]: unit });
  this._unitAdded$.next(unit);
+
+ this.setCurrentUnitForType(unit.getUnitId());
  }
 
  private _unitDisposed$ = new Subject<UnitModel>();
- unitDisposed$ = this._unitDisposed$.asObservable();
+ readonly unitDisposed$ = this._unitDisposed$.asObservable();
  getTypeOfUnitDisposed$<T extends UnitModel<object, number>>(type: UniverInstanceType): Observable<T> {
  return this.unitDisposed$.pipe(filter((unit) => unit.type === type)) as Observable<T>;
  }
@@ -242,10 +256,13 @@ export class UniverInstanceService extends Disposable implements IUniverInstance
  const index = units.indexOf(unit);
  units.splice(index, 1);
 
- this._unitDisposed$.next(unit);
- this._currentUnits$.next({ ...this._currentUnits$.getValue(), [type]: null });
+ // Firstly un-mark the unit as "current".
+ this._removeCurrentUnitForType(type);
  this._focused$.next(null);
 
+ // Then dispose the unit.
+ this._unitDisposed$.next(unit);
+
  return true;
  }
 

packages/core/src/services/permission/permission.service.ts

@@ -25,7 +25,6 @@ export class PermissionService extends Disposable implements IPermissionService
  private _permissionPointMap: Map<string, BehaviorSubject<IPermissionPoint<unknown>>> = new Map();
 
  private _permissionPointUpdate$ = new Subject<IPermissionPoint<unknown>>();
-
  public permissionPointUpdate$ = this._permissionPointUpdate$.asObservable();
 
  deletePermissionPoint(permissionId: string) {

packages/facade/src/apis/__tests__/create-test-bed.ts

@@ -32,7 +32,7 @@ import {
 } from '@univerjs/core';
 import { FormulaDataModel, FunctionService, IFunctionService, LexerTreeBuilder } from '@univerjs/engine-formula';
 import { ISocketService, WebSocketService } from '@univerjs/network';
-import { SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
+import { RangeProtectionRuleModel, SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
 import {
  DescriptionService,
  FormulaCustomFunctionService,
@@ -151,6 +151,7 @@ export function createFacadeTestBed(workbookData?: IWorkbookData, dependencies?:
  injector.add([WorksheetPermissionService]);
  injector.add([WorkbookPermissionService]);
  injector.add([WorksheetProtectionPointModel]);
+ injector.add([RangeProtectionRuleModel]);
  injector.add([IAuthzIoService, { useClass: AuthzIoLocalService }]);
  injector.add([WorksheetProtectionRuleModel]);
 

packages/sheets-formula/src/commands/operations/__tests__/create-command-test-bed.ts

@@ -17,7 +17,7 @@
 import type { IWorkbookData } from '@univerjs/core';
 import { AuthzIoLocalService, IAuthzIoService, ILogService, IUniverInstanceService, LocaleType, LogLevel, Plugin, Univer, UniverInstanceType } from '@univerjs/core';
 import { LexerTreeBuilder } from '@univerjs/engine-formula';
-import { SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
+import { RangeProtectionRuleModel, SelectionManagerService, SheetInterceptorService, WorkbookPermissionService, WorksheetPermissionService, WorksheetProtectionPointModel, WorksheetProtectionRuleModel } from '@univerjs/sheets';
 import type { Dependency } from '@wendellhu/redi';
 import { Inject, Injector } from '@wendellhu/redi';
 import { EditorService, IEditorService } from '@univerjs/ui';
@@ -80,6 +80,7 @@ export function createCommandTestBed(workbookData?: IWorkbookData, dependencies?
  injector.add([WorksheetPermissionService]);
  injector.add([WorksheetProtectionPointModel]);
  injector.add([WorkbookPermissionService]);
+ injector.add([RangeProtectionRuleModel]);
  injector.add([IAuthzIoService, { useClass: AuthzIoLocalService }]);
  injector.add([WorksheetProtectionRuleModel]);
 

packages/sheets-formula/src/controllers/menu.ts

@@ -14,12 +14,13 @@
  * limitations under the License.
  */
 
-import { UniverInstanceType } from '@univerjs/core';
+import type { Workbook } from '@univerjs/core';
+import { IUniverInstanceService, UniverInstanceType } from '@univerjs/core';
 import { getCurrentRangeDisable$, PASTE_SPECIAL_MENU_ID } from '@univerjs/sheets-ui';
 import type { IMenuItem } from '@univerjs/ui';
 import { getMenuHiddenObservable, IClipboardInterfaceService, MenuGroup, MenuItemType, MenuPosition } from '@univerjs/ui';
 import type { IAccessor } from '@wendellhu/redi';
-import { combineLatestWith, map, Observable } from 'rxjs';
+import { combineLatestWith, map, Observable, of, switchMap } from 'rxjs';
 
 import { RangeProtectionPermissionEditPoint, WorkbookEditablePermission, WorksheetEditPermission, WorksheetSetCellValuePermission } from '@univerjs/sheets';
 import { SheetOnlyPasteFormulaCommand } from '../commands/commands/formula-clipboard.command';
@@ -76,8 +77,20 @@ export function MoreFunctionsMenuItemFactory(accessor: IAccessor): IMenuItem {
 }
 
 function menuClipboardDisabledObservable(injector: IAccessor): Observable<boolean> {
- const clipboardDisabled$: Observable<boolean> = new Observable((subscriber) => subscriber.next(!injector.get(IClipboardInterfaceService).supportClipboard));
- return clipboardDisabled$;
+ const univerInstanceService = injector.get(IUniverInstanceService);
+ const workbook$ = univerInstanceService.getCurrentTypeOfUnit$<Workbook>(UniverInstanceType.UNIVER_SHEET);
+ return workbook$.pipe(
+ switchMap((workbook) => {
+ if (!workbook) {
+ return of(true);
+ }
+ const clipboardInterfaceService = injector.get(IClipboardInterfaceService);
+ if (clipboardInterfaceService) {
+ return new Observable((subscriber) => subscriber.next(!injector.get(IClipboardInterfaceService).supportClipboard)) as Observable<boolean>;
+ }
+ return of(true);
+ })
+ );
 }
 
 export function PasteFormulaMenuItemFactory(accessor: IAccessor): IMenuItem {

packages/sheets-thread-comment/src/types/const.ts

@@ -16,4 +16,4 @@
 
 export const SHEETS_THREAD_COMMENT_MODAL = 'univer.sheet.thread-comment-modal';
 export const COMMENT_SINGLE_ICON = 'comment-single';
-export const SHEETS_THREAD_COMMENT = 'univer.sheet.thread-comment';
+export const SHEETS_THREAD_COMMENT = 'SHEET_THREAD_COMMENT';

packages/sheets-ui/src/controllers/menu/__tests__/menu.spec.ts

@@ -93,10 +93,10 @@ describe('test menu items', () => {
  disposableCollection.add(menuItem.activated$!.subscribe((v: boolean) => (activated = v)));
  disposableCollection.add(menuItem.disabled$!.subscribe((v: boolean) => (disabled = v)));
  expect(activated).toBeFalsy();
- expect(disabled).toBeFalsy();
 
  select({ startRow: 0, startColumn: 0, endRow: 0, endColumn: 0 });
  expect(await commandService.executeCommand(SetBoldCommand.id)).toBeTruthy();
  expect(activated).toBe(true);
+ expect(disabled).toBeFalsy();
  });
 });