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

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<string, { color: string }>,
+    textCols: {} as Record<string, Text>,
     rows: {} as Record<string, { color: string }>,
     labels: [] as Array<string>,
   }),
@@ -265,35 +267,20 @@ test('on change', () => {
   const model = block.model as RootModel;
 
   model.title = internalPrimitives.Text('abc');
-  expect(onPropsUpdated).toHaveBeenCalledWith(
+  expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'title');
-    expect.anything(),
-    'title',
-    expect.anything()
-  );
   expect(model.title$.value.toDelta()).toEqual([{ insert: 'abc' }]);
 
   onPropsUpdated.mockClear();
 
   model.title.insert('d', 1);
-  expect(onPropsUpdated).toHaveBeenCalledWith(
+  expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'title');
-    expect.anything(),
-    'title',
-    expect.anything()
-  );
 
   expect(model.title$.value.toDelta()).toEqual([{ insert: 'adbc' }]);
 
   onPropsUpdated.mockClear();
 
   model.boxed.getValue()!.set('foo', 0);
-  expect(onPropsUpdated).toHaveBeenCalledWith(
+  expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'boxed');
-    expect.anything(),
-    'boxed',
-    expect.anything()
-  );
-  expect(onPropsUpdated.mock.calls[0][2].toJSON().value).toMatchObject({
-    foo: 0,
-  });
   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(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'cols');
-    expect.anything(),
-    'cols',
-    expect.anything()
-  );
   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(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'rows');
-    expect.anything(),
-    'rows',
-    expect.anything()
-  );
   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<string>;
   row1.set('color', 'green');
   expect(onRowsUpdated).toHaveBeenCalledWith([{ color: 'green' }]);
-  expect(onPropsUpdated).toHaveBeenCalledWith(
+  expect(onPropsUpdated).toHaveBeenCalledWith(expect.anything(), 'rows');
-    expect.anything(),
-    'rows',
-    expect.anything()
-  );
   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(onChange).toHaveBeenCalledWith(expect.anything(), 'cols');
-      expect.anything(),
-      'cols',
-      expect.anything()
-    );
 
     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(onChange).toHaveBeenCalledWith(expect.anything(), 'cols');
-      expect.anything(),
-      'cols',
-      expect.anything()
-    );
 
     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(onChange).toHaveBeenCalledWith(expect.anything(), 'cols');
-      expect.anything(),
-      'cols',
-      expect.anything()
-    );
 
     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(onChange).toHaveBeenCalledWith(expect.anything(), 'title');
-      expect.anything(),
-      'title',
-      expect.anything()
-    );
 
     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(onChange).toHaveBeenCalledWith(expect.anything(), 'labels');
-      expect.anything(),
-      'labels',
-      expect.anything()
-    );
 
     onChange.mockClear();
     model.props.labels$.value = ['test2'];
     expect(getLabels().toJSON()).toEqual(['test2']);
-    expect(onChange).toHaveBeenCalledWith(
+    expect(onChange).toHaveBeenCalledWith(expect.anything(), 'labels');
-      expect.anything(),
-      'labels',
-      expect.anything()
-    );
 
     onChange.mockClear();
     model.props.labels.splice(0, 1);
     expect(getLabels().toJSON()).toEqual([]);
     expect(model.props.labels$.value).toEqual([]);
-    expect(onChange).toHaveBeenCalledWith(
+    expect(onChange).toHaveBeenCalledWith(expect.anything(), 'labels');
-      expect.anything(),
+
-      'labels',
+    model.props.textCols = {
-      expect.anything()
+      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', () => {

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);

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();
 

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;
               },
             });

blocksuite/framework/store/src/model/block/types.ts

@@ -10,7 +10,7 @@ export type YBlock = Y.Map<unknown> & {
 };
 
 export type BlockOptions = {
-  onChange?: (block: Block, key: string, value: unknown) => void;
+  onChange?: (block: Block, key: string) => void;
 };
 
 export type BlockSysProps = {

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);
-      this._onChange?.(key, value);
     };
   };
 
@@ -471,7 +484,7 @@ export class ReactiveFlatYMap extends BaseReactiveYData<
           }
           this._updateWithSkip(() => {
             proxy[key] = next;
-            this._onChange?.(key, next);
+            this._onChange?.(key);
           });
         })
       );

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;