From 22924c767c75b00a727ae0ce01220306e6b55a70 Mon Sep 17 00:00:00 2001 From: Saul-Mirone Date: Tue, 4 Mar 2025 05:57:05 +0000 Subject: [PATCH] fix(editor): onChange notification for flat model (#10589) The primary purpose of this PR appears to be: 1. Simplifying the change notification API by removing the redundant value parameter from callbacks 2. Improving the reactive system's handling of specialized types (Text and Boxed) in flat data 3. Adding better test coverage for text handling in the flat data model --- .../store/src/__tests__/block.unit.spec.ts | 95 ++++++------------- .../framework/store/src/model/block/block.ts | 4 +- .../src/model/block/flat-sync-controller.ts | 2 +- .../store/src/model/block/sync-controller.ts | 18 ++-- .../framework/store/src/model/block/types.ts | 2 +- .../store/src/reactive/flat-native-y.ts | 33 +++++-- .../tests-legacy/e2e/utils/actions/misc.ts | 2 + 7 files changed, 65 insertions(+), 91 deletions(-) diff --git a/blocksuite/framework/store/src/__tests__/block.unit.spec.ts b/blocksuite/framework/store/src/__tests__/block.unit.spec.ts index 286acfacbd..72b8785b54 100644 --- a/blocksuite/framework/store/src/__tests__/block.unit.spec.ts +++ b/blocksuite/framework/store/src/__tests__/block.unit.spec.ts @@ -10,6 +10,7 @@ import { internalPrimitives, } from '../model/block/index.js'; import type { YBlock } from '../model/block/types.js'; +import type { Text } from '../reactive/text.js'; import { createAutoIncrementIdGenerator } from '../test/index.js'; import { TestWorkspace } from '../test/test-workspace.js'; @@ -47,6 +48,7 @@ const flatTableSchema = defineBlockSchema({ props: internal => ({ title: internal.Text(), cols: { internal: { color: 'white' } } as Record, + textCols: {} as Record, rows: {} as Record, labels: [] as Array, }), @@ -265,35 +267,20 @@ test('on change', () => { const model = block.model as RootModel; model.title = internalPrimitives.Text('abc'); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'title', - expect.anything() - ); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'title'); expect(model.title$.value.toDelta()).toEqual([{ insert: 'abc' }]); onPropsUpdated.mockClear(); model.title.insert('d', 1); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'title', - expect.anything() - ); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'title'); expect(model.title$.value.toDelta()).toEqual([{ insert: 'adbc' }]); onPropsUpdated.mockClear(); model.boxed.getValue()!.set('foo', 0); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'boxed', - expect.anything() - ); - expect(onPropsUpdated.mock.calls[0][2].toJSON().value).toMatchObject({ - foo: 0, - }); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'boxed'); expect(model.boxed$.value.getValue()!.toJSON()).toEqual({ foo: 0, }); @@ -356,11 +343,7 @@ test('deep sync', () => { const map = new Y.Map(); map.set('color', 'green'); getColsMap().set('3', map); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'cols', - expect.anything() - ); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'cols'); expect(onColsUpdated).toHaveBeenCalledWith({ '1': { color: 'red' }, '2': { color: 'blue' }, @@ -373,11 +356,7 @@ test('deep sync', () => { onRowsUpdated.mockClear(); model.rows.push({ color: 'yellow' }); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'rows', - expect.anything() - ); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'rows'); expect(onRowsUpdated).toHaveBeenCalledWith([{ color: 'yellow' }]); expect(onPropsUpdated).toHaveBeenCalledTimes(1); expect(onRowsUpdated).toHaveBeenCalledTimes(1); @@ -388,11 +367,7 @@ test('deep sync', () => { const row1 = getRowsArr().get(0) as Y.Map; row1.set('color', 'green'); expect(onRowsUpdated).toHaveBeenCalledWith([{ color: 'green' }]); - expect(onPropsUpdated).toHaveBeenCalledWith( - expect.anything(), - 'rows', - expect.anything() - ); + expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'rows'); expect(model.rows$.value).toEqual([{ color: 'green' }]); expect(onPropsUpdated).toHaveBeenCalledTimes(1); expect(onRowsUpdated).toHaveBeenCalledTimes(1); @@ -438,11 +413,7 @@ describe('flat', () => { }); expect(onColUpdated).toHaveBeenCalledTimes(1); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'cols', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'cols'); model.props.cols.a.color = 'black'; expect(yBlock.get('prop:cols.a.color')).toBe('black'); @@ -461,11 +432,7 @@ describe('flat', () => { }); expect(onColUpdated).toHaveBeenCalledTimes(1); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'cols', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'cols'); onChange.mockClear(); onColUpdated.mockClear(); @@ -475,11 +442,7 @@ describe('flat', () => { expect(model.props.cols$.value).toEqual({ a: {} }); expect(onColUpdated).toHaveBeenCalledTimes(1); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'cols', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'cols'); model.props.cols = { a: { color: 'red' }, @@ -499,11 +462,7 @@ describe('flat', () => { expect((yBlock.get('prop:title') as Y.Text).toJSON()).toBe('test'); expect(model.props.title$.value.toDelta()).toEqual([{ insert: 'test' }]); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'title', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'title'); onChange.mockClear(); model.props.labels.push('test'); @@ -511,30 +470,30 @@ describe('flat', () => { expect(getLabels().toJSON()).toEqual(['test']); expect(model.props.labels$.value).toEqual(['test']); expect(onChange).toHaveBeenCalledTimes(1); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'labels', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'labels'); onChange.mockClear(); model.props.labels$.value = ['test2']; expect(getLabels().toJSON()).toEqual(['test2']); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'labels', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'labels'); onChange.mockClear(); model.props.labels.splice(0, 1); expect(getLabels().toJSON()).toEqual([]); expect(model.props.labels$.value).toEqual([]); - expect(onChange).toHaveBeenCalledWith( - expect.anything(), - 'labels', - expect.anything() - ); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'labels'); + + model.props.textCols = { + a: internalPrimitives.Text(), + }; + onChange.mockClear(); + model.props.textCols.a.insert('test', 0); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith(expect.anything(), 'textCols'); + expect((yBlock.get('prop:textCols.a') as Y.Text).toJSON()).toBe('test'); + expect(model.props.textCols$.value.a.toDelta()).toEqual([ + { insert: 'test' }, + ]); }); test('stash and pop', () => { diff --git a/blocksuite/framework/store/src/model/block/block.ts b/blocksuite/framework/store/src/model/block/block.ts index 0b091404c0..60c2d70118 100644 --- a/blocksuite/framework/store/src/model/block/block.ts +++ b/blocksuite/framework/store/src/model/block/block.ts @@ -43,11 +43,11 @@ export class Block { ) { const onChange = !options.onChange ? undefined - : (key: string, value: unknown) => { + : (key: string) => { if (!this._syncController || !this.model) { return; } - options.onChange?.(this, key, value); + options.onChange?.(this, key); }; const flavour = yBlock.get('sys:flavour') as string; const blockSchema = this.schema.get(flavour); diff --git a/blocksuite/framework/store/src/model/block/flat-sync-controller.ts b/blocksuite/framework/store/src/model/block/flat-sync-controller.ts index 72887d40ab..7c58734bb6 100644 --- a/blocksuite/framework/store/src/model/block/flat-sync-controller.ts +++ b/blocksuite/framework/store/src/model/block/flat-sync-controller.ts @@ -25,7 +25,7 @@ export class FlatSyncController { readonly schema: Schema, readonly yBlock: YBlock, readonly doc?: Store, - readonly onChange?: (key: string, value: unknown) => void + readonly onChange?: (key: string) => void ) { const { id, flavour, version, yChildren, props } = this._parseYBlock(); diff --git a/blocksuite/framework/store/src/model/block/sync-controller.ts b/blocksuite/framework/store/src/model/block/sync-controller.ts index c77f792a01..50c817e5b6 100644 --- a/blocksuite/framework/store/src/model/block/sync-controller.ts +++ b/blocksuite/framework/store/src/model/block/sync-controller.ts @@ -57,7 +57,7 @@ export class SyncController { } }); }); - this.onChange?.(keyName, value); + this.onChange?.(keyName); return; } if (type.action === 'delete') { @@ -70,7 +70,7 @@ export class SyncController { this.model[`${keyName}$`].value = undefined; } }); - this.onChange?.(keyName, undefined); + this.onChange?.(keyName); return; } }); @@ -105,7 +105,7 @@ export class SyncController { readonly schema: Schema, readonly yBlock: YBlock, readonly doc?: Store, - readonly onChange?: (key: string, value: unknown) => void + readonly onChange?: (key: string) => void ) { const { id, flavour, version, yChildren, props } = this._parseYBlock(); @@ -175,7 +175,7 @@ export class SyncController { if (this._stashed.has(p)) { setValue(target, p, value); const result = Reflect.set(target, p, value, receiver); - this.onChange?.(p, value); + this.onChange?.(p); return result; } @@ -220,7 +220,7 @@ export class SyncController { private _getPropsProxy(name: string, value: unknown) { return createYProxy(value, { onChange: () => { - this.onChange?.(name, value); + this.onChange?.(name); const signalKey = `${name}$`; if (signalKey in this.model) { this._mutex(() => { @@ -339,12 +339,12 @@ export class SyncController { }, set: (target, p, value, receiver) => { const result = Reflect.set(target, p, value, receiver); - this.onChange?.(prop, value); + this.onChange?.(prop); return result; }, deleteProperty: (target, p) => { const result = Reflect.deleteProperty(target, p); - this.onChange?.(prop, undefined); + this.onChange?.(prop); return result; }, }); @@ -360,12 +360,12 @@ export class SyncController { return Reflect.set(target, p, value, receiver); } const result = Reflect.set(target, p, value, receiver); - this.onChange?.(prop, value); + this.onChange?.(prop); return result; }, deleteProperty: (target, p) => { const result = Reflect.deleteProperty(target, p); - this.onChange?.(p as string, undefined); + this.onChange?.(p as string); return result; }, }); diff --git a/blocksuite/framework/store/src/model/block/types.ts b/blocksuite/framework/store/src/model/block/types.ts index 7224b4e5d3..35a52b5e65 100644 --- a/blocksuite/framework/store/src/model/block/types.ts +++ b/blocksuite/framework/store/src/model/block/types.ts @@ -10,7 +10,7 @@ export type YBlock = Y.Map & { }; export type BlockOptions = { - onChange?: (block: Block, key: string, value: unknown) => void; + onChange?: (block: Block, key: string) => void; }; export type BlockSysProps = { diff --git a/blocksuite/framework/store/src/reactive/flat-native-y.ts b/blocksuite/framework/store/src/reactive/flat-native-y.ts index af19d3f7c3..4b459e1c66 100644 --- a/blocksuite/framework/store/src/reactive/flat-native-y.ts +++ b/blocksuite/framework/store/src/reactive/flat-native-y.ts @@ -22,13 +22,13 @@ const keyWithoutPrefix = (key: string) => key.replace(/(prop|sys):/, ''); const keyWithPrefix = (key: string) => SYS_KEYS.has(key) ? `sys:${key}` : `prop:${key}`; -type OnChange = (key: string, value: unknown) => void; +type OnChange = (key: string) => void; type Transform = (key: string, value: unknown, origin: unknown) => unknown; type CreateProxyOptions = { basePath?: string; onChange?: OnChange; - transform?: Transform; + transform: Transform; onDispose: Slot; shouldByPassSignal: () => boolean; shouldByPassYjs: () => boolean; @@ -64,7 +64,7 @@ function createProxy( basePath, onChange, initialized, - transform = (_key, value) => value, + transform, stashed, } = options; const isRoot = !basePath; @@ -120,7 +120,7 @@ function createProxy( } byPassSignalUpdate(() => { proxy[p] = next; - onChange?.(firstKey, next); + onChange?.(firstKey); }); }) ); @@ -137,7 +137,7 @@ function createProxy( : prev; // @ts-expect-error allow magic props root[signalKey].value = next; - onChange?.(firstKey, next); + onChange?.(firstKey); }); }; @@ -158,6 +158,11 @@ function createProxy( run(value, fullPath); } else { list.push(() => { + if (value instanceof Text || Boxed.is(value)) { + value.bind(() => { + onChange?.(firstKey); + }); + } yMap.set(keyWithPrefix(fullPath), native2Y(value)); }); } @@ -188,6 +193,11 @@ function createProxy( return result; } + if (value instanceof Text || Boxed.is(value)) { + value.bind(() => { + onChange?.(firstKey); + }); + } const yValue = native2Y(value); const next = transform(firstKey, value, yValue); if (!isStashed && initialized() && !shouldByPassYjs()) { @@ -239,7 +249,7 @@ function createProxy( : prev; // @ts-expect-error allow magic props root[signalKey].value = next; - onChange?.(firstKey, next); + onChange?.(firstKey); }); }; @@ -303,7 +313,11 @@ export class ReactiveFlatYMap extends BaseReactiveYData< acc[key] = {}; } if (index === arr.length - 1) { - acc[key] = y2Native(value); + acc[key] = y2Native(value, { + transform: (value, origin) => { + return this._transform(firstKey, value, origin); + }, + }); } return acc[key] as UnRecord; }, proxy as UnRecord); @@ -375,8 +389,7 @@ export class ReactiveFlatYMap extends BaseReactiveYData< private readonly _getPropOnChange = (key: string) => { return () => { - const value = this._proxy[key]; - this._onChange?.(key, value); + this._onChange?.(key); }; }; @@ -471,7 +484,7 @@ export class ReactiveFlatYMap extends BaseReactiveYData< } this._updateWithSkip(() => { proxy[key] = next; - this._onChange?.(key, next); + this._onChange?.(key); }); }) ); diff --git a/blocksuite/tests-legacy/e2e/utils/actions/misc.ts b/blocksuite/tests-legacy/e2e/utils/actions/misc.ts index 728f47a7c7..210ea7b214 100644 --- a/blocksuite/tests-legacy/e2e/utils/actions/misc.ts +++ b/blocksuite/tests-legacy/e2e/utils/actions/misc.ts @@ -146,6 +146,8 @@ export async function enterPlaygroundRoom( 'Lit is in dev mode. Not recommended for production! See https://lit.dev/msg/dev-mode for more information.', // Figma embed: 'Running frontend commit', + // Github timeout: + 'Failed to load resource: the server responded with a status of 403', ].some(text => message.text().startsWith(text)) ) { return;