From 630c3242de52138e079715b19c0d27e1bb1908b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=99=BD=E7=86=B1?= Date: Wed, 13 May 2026 22:05:45 +0800 Subject: [PATCH] fix: properly dispose RxJS subscriptions to prevent memory leaks (#6896) --- .github/workflows/release.yml | 21 ++++++----- common/shared/package.json | 2 +- packages/core/src/shared/rxjs.ts | 7 +++- .../src/services/find-replace.service.ts | 9 ++++- .../src/controllers/cf.viewport.controller.ts | 28 ++++++++++---- .../package.json | 2 +- .../src/facade/f-text-finder.ts | 37 +++++-------------- .../sheet.render-controller.ts | 20 +++++----- .../skeleton.render-controller.ts | 8 ++-- .../src/model/range-protection.cache.ts | 20 +++++----- pnpm-lock.yaml | 14 +++---- 11 files changed, 89 insertions(+), 79 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a6899dbdb3..a6aa3893ca 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -33,12 +33,14 @@ jobs: - name: 🚚 Get release type id: release-type + env: + REF_NAME: ${{ github.ref_name }} run: | - if [[ ${{ github.ref_name }} =~ -(alpha|beta|rc)\. ]]; then + if [[ "$REF_NAME" =~ -(alpha|beta|rc)\. ]]; then extracted_type="${BASH_REMATCH[1]}" - echo "value=$extracted_type" >> $GITHUB_OUTPUT + echo "value=$extracted_type" >> "$GITHUB_OUTPUT" else - echo "value=stable" >> $GITHUB_OUTPUT + echo "value=stable" >> "$GITHUB_OUTPUT" fi release-npm: @@ -73,12 +75,13 @@ jobs: pnpm build - name: 🐙 Publish - run: | - if [[ ${{ needs.prepare.outputs.release_type }} == "stable" ]]; then - pnpm publish --access public -r --no-git-checks - else - pnpm publish --access public --tag ${{ needs.prepare.outputs.release_type }} -r --no-git-checks - fi env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} NPM_CONFIG_PROVENANCE: true + RELEASE_TYPE: ${{ needs.prepare.outputs.release_type }} + run: | + if [[ "$RELEASE_TYPE" == "stable" ]]; then + pnpm publish --access public -r --no-git-checks + else + pnpm publish --access public --tag "$RELEASE_TYPE" -r --no-git-checks + fi diff --git a/common/shared/package.json b/common/shared/package.json index 2b88c3d27f..77bca377e4 100644 --- a/common/shared/package.json +++ b/common/shared/package.json @@ -52,7 +52,7 @@ "devDependencies": { "@types/fs-extra": "^11.0.4", "@univerjs/icons": "^1.2.0", - "@univerjs/icons-svg": "^1.1.1", + "@univerjs/icons-svg": "^1.2.0", "@univerjs/protocol": "workspace:*", "typescript": "^6.0.3", "vue-tsc": "^3.2.8" diff --git a/packages/core/src/shared/rxjs.ts b/packages/core/src/shared/rxjs.ts index 0f279bf591..b4ad4c0983 100644 --- a/packages/core/src/shared/rxjs.ts +++ b/packages/core/src/shared/rxjs.ts @@ -80,7 +80,12 @@ export function afterTime(ms: number): Observable { export function convertObservableToBehaviorSubject(observable: Observable, initValue: T): BehaviorSubject { const subject = new BehaviorSubject(initValue); - observable.subscribe(subject); + const subscription = observable.subscribe(subject); + const originalComplete = subject.complete.bind(subject); + subject.complete = () => { + subscription.unsubscribe(); + originalComplete(); + }; return subject; } diff --git a/packages/find-replace/src/services/find-replace.service.ts b/packages/find-replace/src/services/find-replace.service.ts index 282035feef..9b78d044d8 100644 --- a/packages/find-replace/src/services/find-replace.service.ts +++ b/packages/find-replace/src/services/find-replace.service.ts @@ -688,6 +688,7 @@ export class FindReplaceService extends Disposable implements IFindReplaceServic private readonly _providers = new Set(); private readonly _state = new FindReplaceState(); private _model: Nullable; + private _modelDisposables: Nullable = null; private readonly _currentMatch$ = new BehaviorSubject>(null); readonly currentMatch$ = this._currentMatch$.asObservable(); @@ -833,8 +834,9 @@ export class FindReplaceService extends Disposable implements IFindReplaceServic } this._model = this._injector.createInstance(FindReplaceModel, this._state, this._providers); - this._model.currentMatch$.subscribe((match) => this._currentMatch$.next(match)); - this._model.replaceables$.subscribe((replaceables) => this._replaceables$.next(replaceables)); + this._modelDisposables = new DisposableCollection(); + this._modelDisposables.add(toDisposable(this._model.currentMatch$.subscribe((match) => this._currentMatch$.next(match)))); + this._modelDisposables.add(toDisposable(this._model.replaceables$.subscribe((replaceables) => this._replaceables$.next(replaceables)))); const newState = createInitFindReplaceState(); if (revealReplace) { @@ -854,6 +856,9 @@ export class FindReplaceService extends Disposable implements IFindReplaceServic this._model?.dispose(); this._model = null; + this._modelDisposables?.dispose(); + this._modelDisposables = null; + this._toggleDisplayRawFormula(false); this._toggleRevealReplace(false); } diff --git a/packages/sheets-conditional-formatting-ui/src/controllers/cf.viewport.controller.ts b/packages/sheets-conditional-formatting-ui/src/controllers/cf.viewport.controller.ts index 28e2f6b45f..56550400d3 100644 --- a/packages/sheets-conditional-formatting-ui/src/controllers/cf.viewport.controller.ts +++ b/packages/sheets-conditional-formatting-ui/src/controllers/cf.viewport.controller.ts @@ -15,12 +15,14 @@ */ import type { Workbook } from '@univerjs/core'; -import { Disposable, Inject, IUniverInstanceService, UniverInstanceType } from '@univerjs/core'; +import { Disposable, DisposableCollection, Inject, IUniverInstanceService, UniverInstanceType } from '@univerjs/core'; import { IRenderManagerService } from '@univerjs/engine-render'; import { CONDITIONAL_FORMATTING_VIEWPORT_CACHE_LENGTH, ConditionalFormattingViewModel } from '@univerjs/sheets-conditional-formatting'; import { SheetSkeletonManagerService } from '@univerjs/sheets-ui'; export class ConditionalFormattingViewportController extends Disposable { + private _unitDisposable: DisposableCollection = new DisposableCollection(); + constructor( @Inject(ConditionalFormattingViewModel) private _conditionalFormattingViewModel: ConditionalFormattingViewModel, @IUniverInstanceService private _univerInstanceService: IUniverInstanceService, @@ -33,13 +35,15 @@ export class ConditionalFormattingViewportController extends Disposable { private _init() { const unit = this._univerInstanceService.getCurrentUnitForType(UniverInstanceType.UNIVER_SHEET); const bindUnit = (unit: Workbook) => { + this._unitDisposable.dispose(); + this._unitDisposable = new DisposableCollection(); const unitId = unit.getUnitId(); const render = this._renderManagerService.getRenderById(unitId); if (!render) { return; } const sheetSkeletonManagerService = render.with(SheetSkeletonManagerService); - this.disposeWithMe(sheetSkeletonManagerService.currentSkeleton$.subscribe((s) => { + this._unitDisposable.add(sheetSkeletonManagerService.currentSkeleton$.subscribe((s) => { if (s) { const range = s.skeleton.rowColumnSegment; const col = range.endColumn - range.startColumn + 1; @@ -54,11 +58,19 @@ export class ConditionalFormattingViewportController extends Disposable { if (unit) { bindUnit(unit); } - this._univerInstanceService.getCurrentTypeOfUnit$(UniverInstanceType.UNIVER_SHEET).subscribe((unit) => { - if (!unit) { - return; - } - bindUnit(unit); - }); + this.disposeWithMe( + this._univerInstanceService.getCurrentTypeOfUnit$(UniverInstanceType.UNIVER_SHEET).subscribe((unit) => { + if (!unit) { + this._unitDisposable.dispose(); + return; + } + bindUnit(unit); + }) + ); + } + + override dispose(): void { + this._unitDisposable.dispose(); + super.dispose(); } } diff --git a/packages/sheets-conditional-formatting/package.json b/packages/sheets-conditional-formatting/package.json index 2d08d4f044..546ad6c827 100644 --- a/packages/sheets-conditional-formatting/package.json +++ b/packages/sheets-conditional-formatting/package.json @@ -86,7 +86,7 @@ }, "devDependencies": { "@univerjs-infra/shared": "workspace:*", - "@univerjs/icons-svg": "^1.1.1", + "@univerjs/icons-svg": "^1.2.0", "rxjs": "^7.8.2", "typescript": "^6.0.3", "vitest": "^4.1.5" diff --git a/packages/sheets-find-replace/src/facade/f-text-finder.ts b/packages/sheets-find-replace/src/facade/f-text-finder.ts index 6c3c5ac2fe..0ddc5b5129 100644 --- a/packages/sheets-find-replace/src/facade/f-text-finder.ts +++ b/packages/sheets-find-replace/src/facade/f-text-finder.ts @@ -19,6 +19,7 @@ import type { IFindComplete, IFindMatch, IFindReplaceState } from '@univerjs/fin import { Disposable, Inject, Injector, IUniverInstanceService } from '@univerjs/core'; import { createInitFindReplaceState, FindBy, FindReplaceModel, FindReplaceState, IFindReplaceService } from '@univerjs/find-replace'; import { FRange } from '@univerjs/sheets/facade'; +import { filter, firstValueFrom } from 'rxjs'; /** * @ignore @@ -412,41 +413,23 @@ export class FTextFinder extends Disposable implements IFTextFinder { async matchCaseAsync(matchCase: boolean): Promise { this._state.changeState({ caseSensitive: matchCase, findCompleted: false }); - return new Promise((resolve) => { - const subscribe = this._state.stateUpdates$.subscribe(async (state) => { - if (state.findCompleted === true) { - subscribe.unsubscribe(); - await this.ensureCompleteAsync(); - resolve(this); - } - }); - }); + await firstValueFrom(this._state.stateUpdates$.pipe(filter((state) => state.findCompleted === true))); + await this.ensureCompleteAsync(); + return this; } async matchEntireCellAsync(matchEntireCell: boolean): Promise { this._state.changeState({ matchesTheWholeCell: matchEntireCell, findCompleted: false }); - return new Promise((resolve) => { - const subscribe = this._state.stateUpdates$.subscribe(async (state) => { - if (state.findCompleted === true) { - subscribe.unsubscribe(); - await this.ensureCompleteAsync(); - resolve(this); - } - }); - }); + await firstValueFrom(this._state.stateUpdates$.pipe(filter((state) => state.findCompleted === true))); + await this.ensureCompleteAsync(); + return this; } async matchFormulaTextAsync(matchFormulaText: boolean): Promise { this._state.changeState({ findBy: matchFormulaText ? FindBy.FORMULA : FindBy.VALUE, findCompleted: false }); - return new Promise((resolve) => { - const subscribe = this._state.stateUpdates$.subscribe(async (state) => { - if (state.findCompleted === true) { - subscribe.unsubscribe(); - await this.ensureCompleteAsync(); - resolve(this); - } - }); - }); + await firstValueFrom(this._state.stateUpdates$.pipe(filter((state) => state.findCompleted === true))); + await this.ensureCompleteAsync(); + return this; } async replaceAllWithAsync(replaceText: string): Promise { diff --git a/packages/sheets-ui/src/controllers/render-controllers/sheet.render-controller.ts b/packages/sheets-ui/src/controllers/render-controllers/sheet.render-controller.ts index 17d1c345e4..b3c48c6b2f 100644 --- a/packages/sheets-ui/src/controllers/render-controllers/sheet.render-controller.ts +++ b/packages/sheets-ui/src/controllers/render-controllers/sheet.render-controller.ts @@ -108,12 +108,12 @@ export class SheetRenderController extends RxDisposable implements IRenderModule private _initRenderMetricSubscriber() { const { engine } = this._context; - engine.beginFrame$.subscribe(() => { + this.disposeWithMe(engine.beginFrame$.subscribe(() => { this._renderFrameTimeMetric = null; this._renderFrameTags = {}; - }); + })); - engine.endFrame$.subscribe(() => { + this.disposeWithMe(engine.endFrame$.subscribe(() => { const validRenderInfo = this._renderFrameTimeMetric && Object.keys(this._renderFrameTimeMetric).filter((key) => key.startsWith(SHEET_EXTENSION_PREFIX)).length > 0; @@ -123,22 +123,22 @@ export class SheetRenderController extends RxDisposable implements IRenderModule tags: this._renderFrameTags, } as IAfterRender$Info); } - }); + })); - engine.renderFrameTimeMetric$.subscribe(([key, value]: ITimeMetric) => { + this.disposeWithMe(engine.renderFrameTimeMetric$.subscribe(([key, value]: ITimeMetric) => { if (!this._renderFrameTimeMetric) this._renderFrameTimeMetric = {}; if (!this._renderFrameTimeMetric[key]) { this._renderFrameTimeMetric[key] = []; } this._renderFrameTimeMetric[key].push(Math.round(value * 100) / 100); - }); + })); - engine.renderFrameTags$.subscribe(([key, value]: [string, any]) => { + this.disposeWithMe(engine.renderFrameTags$.subscribe(([key, value]: [string, any]) => { this._renderFrameTags[key] = value; - }); + })); const frameInfoList: IExtendFrameInfo[] = []; - this._afterRenderMetric$.pipe(withLatestFrom(engine.endFrame$)).subscribe(([sceneRenderDetail, basicFrameTimeInfo]: [IAfterRender$Info, IBasicFrameInfo]) => { + this.disposeWithMe(this._afterRenderMetric$.pipe(withLatestFrom(engine.endFrame$)).subscribe(([sceneRenderDetail, basicFrameTimeInfo]: [IAfterRender$Info, IBasicFrameInfo]) => { frameInfoList.push({ ...{ FPS: basicFrameTimeInfo.FPS, @@ -152,7 +152,7 @@ export class SheetRenderController extends RxDisposable implements IRenderModule this._captureRenderMetric(frameInfoList); frameInfoList.length = 0; } - }); + })); } /** diff --git a/packages/sheets-ui/src/controllers/render-controllers/skeleton.render-controller.ts b/packages/sheets-ui/src/controllers/render-controllers/skeleton.render-controller.ts index 07f29ec317..488d785da8 100644 --- a/packages/sheets-ui/src/controllers/render-controllers/skeleton.render-controller.ts +++ b/packages/sheets-ui/src/controllers/render-controllers/skeleton.render-controller.ts @@ -29,9 +29,11 @@ export class SheetSkeletonRenderController extends Disposable implements IRender ) { super(); - this._sheetSkeletonManagerService.currentSkeleton$.subscribe((param: Nullable) => { - this._updateSceneSize(param); - }); + this.disposeWithMe( + this._sheetSkeletonManagerService.currentSkeleton$.subscribe((param: Nullable) => { + this._updateSceneSize(param); + }) + ); } private _updateSceneSize(param: Nullable) { diff --git a/packages/sheets/src/model/range-protection.cache.ts b/packages/sheets/src/model/range-protection.cache.ts index 80fdee5311..04d30faf16 100644 --- a/packages/sheets/src/model/range-protection.cache.ts +++ b/packages/sheets/src/model/range-protection.cache.ts @@ -56,7 +56,7 @@ export class RangeProtectionCache extends Disposable { } private _initUpdateCellInfoCache() { - this._permissionService.permissionPointUpdate$.pipe( + this.disposeWithMe(this._permissionService.permissionPointUpdate$.pipe( filter((permission) => permission.type === UnitObject.SelectRange), map((permission) => permission as IRangePermissionPoint) ).subscribe((permission) => { @@ -79,9 +79,9 @@ export class RangeProtectionCache extends Disposable { } } }); - }); + })); - this._ruleModel.ruleChange$.subscribe((info) => { + this.disposeWithMe(this._ruleModel.ruleChange$.subscribe((info) => { const { unitId, subUnitId } = info; const cellInfoMap = this._ensureCellInfoMap(unitId, subUnitId); info.rule.ranges.forEach((range) => { @@ -96,11 +96,11 @@ export class RangeProtectionCache extends Disposable { }); }); } - }); + })); } private _initUpdateCellRuleCache() { - this._ruleModel.ruleChange$.subscribe((ruleChange) => { + this.disposeWithMe(this._ruleModel.ruleChange$.subscribe((ruleChange) => { const { type } = ruleChange; if (type === 'add') { this._addCellRuleCache(ruleChange); @@ -110,7 +110,7 @@ export class RangeProtectionCache extends Disposable { this._deleteCellRuleCache({ ...ruleChange, rule: ruleChange.oldRule! }); this._addCellRuleCache(ruleChange); } - }); + })); } private _ensureRuleMap(unitId: string, subUnitId: string) { @@ -285,7 +285,7 @@ export class RangeProtectionCache extends Disposable { } private _initUpdateRowColInfoCache() { - this._permissionService.permissionPointUpdate$.pipe( + this.disposeWithMe(this._permissionService.permissionPointUpdate$.pipe( filter((permission) => permission.type === UnitObject.SelectRange), map((permission) => permission as IRangePermissionPoint) ).subscribe({ @@ -325,9 +325,9 @@ export class RangeProtectionCache extends Disposable { } }); }, - }); + })); - this._ruleModel.ruleChange$.subscribe((info) => { + this.disposeWithMe(this._ruleModel.ruleChange$.subscribe((info) => { if (info.type === 'delete') { const { unitId, subUnitId, rule } = info; const rowInfoMap = this._ensureRowColInfoMap(unitId, subUnitId, 'row'); @@ -345,7 +345,7 @@ export class RangeProtectionCache extends Disposable { } }); } - }); + })); } public getCellInfo(unitId: string, subUnitId: string, row: number, col: number) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index af3161f069..d721f03ace 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -382,8 +382,8 @@ importers: specifier: ^1.2.0 version: 1.2.0(react-dom@19.2.6(react@19.2.6))(react@19.2.6) '@univerjs/icons-svg': - specifier: ^1.1.1 - version: 1.1.1 + specifier: ^1.2.0 + version: 1.2.0 '@univerjs/protocol': specifier: workspace:* version: link:../../packages/protocol @@ -1602,8 +1602,8 @@ importers: specifier: workspace:* version: link:../../common/shared '@univerjs/icons-svg': - specifier: ^1.1.1 - version: 1.1.1 + specifier: ^1.2.0 + version: 1.2.0 rxjs: specifier: ^7.8.2 version: 7.8.2 @@ -6385,8 +6385,8 @@ packages: resolution: {integrity: sha512-NwjLUnGy8/Zfx23fl50tRC8rYaYnM52xNRYFAXvmiil9yh1+K6aRVQMnzW6gQB/1DLgWt977lYQn7C+wtgXZiA==} engines: {node: ^18.18.0 || ^20.9.0 || >=21.1.0} - '@univerjs/icons-svg@1.1.1': - resolution: {integrity: sha512-ur59SIR6JViQGFCwHfXIURmV5qBbSKcvE9OF9BRfDvxphmXYtWwfC0/ubrWF5Z4RGNKtP3VTPiVSJT27lkfE3A==} + '@univerjs/icons-svg@1.2.0': + resolution: {integrity: sha512-Tm0cYbTQwP2VfpiZClyzff5HO7w9UCZ/Qi0YWZ/YWi16QBTLqsc47a80VPAZTwOgUPqejuGxpBKViz6Welht6w==} '@univerjs/icons@1.2.0': resolution: {integrity: sha512-+rw4sSNGjyn4FI6LWa2Z7cMtc5MzBMr7cAaAfeh66d/aqT60CCrBbuF3x+nuC/fvDMXQzEjOqywXUZ4HE+MwLw==} @@ -14487,7 +14487,7 @@ snapshots: '@typescript-eslint/types': 8.59.2 eslint-visitor-keys: 5.0.1 - '@univerjs/icons-svg@1.1.1': {} + '@univerjs/icons-svg@1.2.0': {} '@univerjs/icons@1.2.0(react-dom@19.2.6(react@19.2.6))(react@19.2.6)': dependencies: