feat(infra): doc properties by orm #8382

EYHN · 2024-09-25T02:32:36Z

create new orm table docConfiguration

move primary store to docConfiguration

graphite-app · 2024-09-25T02:32:41Z

Your org has enabled the Graphite merge queue for merging into canary

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

EYHN · 2024-09-25T02:32:55Z

feat(infra): doc properties by orm #8382 👈
canary

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @EYHN and the rest of your teammates on Graphite

nx-cloud · 2024-09-25T02:33:09Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c4e599c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 6 targets

Sent with 💌 from NxCloud.

codecov · 2024-09-25T02:36:14Z

Codecov Report

Attention: Patch coverage is 14.87603% with 103 lines in your changes missing coverage. Please review.

Project coverage is 70.07%. Comparing base (4e30f75) to head (c4e599c).
Report is 2 commits behind head on canary.

Files with missing lines	Patch %	Lines
...mon/infra/src/modules/doc/stores/doc-properties.ts	0.00%	45 Missing ⚠️
packages/common/infra/src/utils/yjs-observable.ts	40.00%	24 Missing and 3 partials ⚠️
...ckages/common/infra/src/modules/doc/stores/docs.ts	0.00%	25 Missing ⚠️
...on/infra/src/modules/doc/entities/property-list.ts	0.00%	4 Missing ⚠️
...es/common/infra/src/modules/doc/entities/record.ts	0.00%	1 Missing ⚠️
.../infra/src/modules/workspace/entities/workspace.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           canary    #8382      +/-   ##
==========================================
- Coverage   70.21%   70.07%   -0.14%     
==========================================
  Files         520      523       +3     
  Lines       32961    33051      +90     
  Branches     2879     2902      +23     
==========================================
+ Hits        23142    23161      +19     
- Misses       9481     9549      +68     
- Partials      338      341       +3

Flag	Coverage Δ
server-test	`76.71% <ø> (ø)`
unittest	`45.71% <14.87%> (-0.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pengx17 · 2024-09-27T09:49:22Z

packages/common/infra/src/utils/yjs-observable.ts

+ * @example
+ * yjsObserveDeep(yjs) -> emit when any of its deep children changed
+ */
+export function yjsObserveDeep(yjs?: any) {


@Saul-Mirone can you help review this function?

pengx17 · 2024-09-27T10:34:49Z

packages/common/infra/src/modules/doc/stores/doc-properties.ts

+  updateDocPropertyInfo(id: string, config: Partial<DocCustomPropertyInfo>) {
+    const needMigration = !this.dbService.db.docCustomPropertyInfo.get(id);
+    if (needMigration) {
+      // if this property is not in db, we need to migration it from latency to db, only type and name is needed
+      const latencyId = id.replace('custom:', '');
+      const latency = this.getLatencyDocPropertyInfo(latencyId);
+      this.dbService.db.docCustomPropertyInfo.create({
+        id,
+        type:
+          latency?.type ??
+          'unknown' /* should never reach here, just for safety, we need handle unknown property type */,
+        name: latency?.name,
+        ...config,
+      });
+      return;
+    } else {
+      this.dbService.db.docCustomPropertyInfo.update(id, config);
+    }
+  }


may split the migration code for clearity

Suggested change

updateDocPropertyInfo(id: string, config: Partial<DocCustomPropertyInfo>) {

const needMigration = !this.dbService.db.docCustomPropertyInfo.get(id);

if (needMigration) {

// if this property is not in db, we need to migration it from latency to db, only type and name is needed

const latencyId = id.replace('custom:', '');

const latency = this.getLatencyDocPropertyInfo(latencyId);

this.dbService.db.docCustomPropertyInfo.create({

id,

type:

latency?.type ??

'unknown' /* should never reach here, just for safety, we need handle unknown property type */,

name: latency?.name,

...config,

});

return;

} else {

this.dbService.db.docCustomPropertyInfo.update(id, config);

}

}

updateDocPropertyInfo(id: string, config: Partial<DocCustomPropertyInfo>) {

const needMigration = !this.dbService.db.docCustomPropertyInfo.get(id);

return needMigration

? this.migrateAndCreatePropertyInfo(id, config)

: this.dbService.db.docCustomPropertyInfo.update(id, config);

}

private migrateAndCreatePropertyInfo(id: string, config: Partial<DocCustomPropertyInfo>) {

const latencyId = id.replace('custom:', '');

const latency = this.getLatencyDocPropertyInfo(latencyId);

return this.dbService.db.docCustomPropertyInfo.create({

id,

type: latency?.type ?? 'unknown',

name: latency?.name,

...config,

});

}

pengx17 · 2024-09-27T10:36:14Z

packages/common/infra/src/modules/doc/stores/doc-properties.ts

+} from '../../db/schema/schema';
+import type { WorkspaceService } from '../../workspace';
+
+interface LatencyDocProperties {


Do you mean LegacyDocProperties?

pengx17 · 2024-09-27T10:50:07Z

packages/common/infra/src/utils/yjs-observable.ts

+  if (target instanceof YArray) {
+    return new Observable(subscriber => {
+      const refresh = () => {
+        if ('index' in current) {
+          subscriber.next(target.get(current.index));
+        } else {
+          subscriber.next(undefined);
+        }
+      };
+      target.observe(refresh);
+      return () => {
+        target.unobserve(refresh);
+      };
+    }).pipe(
+      distinctUntilChanged(),
+      switchMap(arr => _yjsDeepWatch(arr, path.slice(1)))
+    );
+  } else if (target instanceof YMap) {
+    return new Observable(subscriber => {
+      const refresh = () => {
+        if ('key' in current) {
+          subscriber.next(target.get(current.key));
+        } else {
+          subscriber.next(undefined);
+        }
+      };
+      refresh();
+      target.observe(refresh);
+      return () => {
+        target.unobserve(refresh);
+      };
+    }).pipe(
+      distinctUntilChanged(),
+      switchMap(arr => _yjsDeepWatch(arr, path.slice(1)))
+    );


the two branches are almost the same. perhaps merge them?

pengx17 · 2024-09-27T11:00:51Z

packages/common/infra/src/utils/yjs-observable.ts

+          subscriber.next(undefined);
+        }
+      };
+      target.observe(refresh);


refresh on init?

pengx17 · 2024-09-27T11:06:50Z

packages/common/infra/src/utils/yjs-observable.ts

+ */
+export function yjsObserveByPath(yjs: YAbstractType<any>, path: string) {
+  const parsedPath = parsePath(path);
+  return _yjsDeepWatch(yjs, parsedPath);


What if the target being watched hasn't been created yet at the time of observation? Should we also observe its parent in this case?

We will observe the parent until the child is created

EYHN marked this pull request as ready for review September 25, 2024 02:32

graphite-app bot requested review from CatsJuice, JimmFly, pengx17 and forehalo September 25, 2024 02:32

github-actions bot added the mod:infra Environment related issues and discussions label Sep 25, 2024

pengx17 approved these changes Sep 25, 2024

View reviewed changes

EYHN marked this pull request as draft September 26, 2024 16:57

EYHN force-pushed the eyhn/doc-configuration branch from b4af180 to 3ffc832 Compare September 27, 2024 09:32

EYHN changed the title ~~feat(infra): doc configuration~~ feat(infra): doc properties by orm Sep 27, 2024

github-actions bot added the test Related to test cases label Sep 27, 2024

EYHN marked this pull request as ready for review September 27, 2024 09:32

EYHN requested a review from pengx17 September 27, 2024 09:32

pengx17 reviewed Sep 27, 2024

View reviewed changes

EYHN force-pushed the eyhn/doc-configuration branch 2 times, most recently from da29f9f to d28dece Compare September 27, 2024 09:56

pengx17 reviewed Sep 27, 2024

View reviewed changes

feat(infra): doc configuration

c4e599c

EYHN force-pushed the eyhn/doc-configuration branch from d28dece to c4e599c Compare September 27, 2024 14:02

CatsJuice approved these changes Sep 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(infra): doc properties by orm #8382

feat(infra): doc properties by orm #8382

feat(infra): doc properties by orm #8382

Are you sure you want to change the base?

feat(infra): doc properties by orm #8382

Conversation

Your org has enabled the Graphite merge queue for merging into canary

☁️ Nx Cloud Report

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment