Skip to content

refactor(tree): clarify move logic#27466

Open
yann-achard-MS wants to merge 8 commits into
microsoft:mainfrom
yann-achard-MS:cleanup-move-logic
Open

refactor(tree): clarify move logic#27466
yann-achard-MS wants to merge 8 commits into
microsoft:mainfrom
yann-achard-MS:cleanup-move-logic

Conversation

@yann-achard-MS
Copy link
Copy Markdown
Contributor

@yann-achard-MS yann-achard-MS commented Jun 2, 2026 •
edited
Loading

Description

Clarifies the logic of DefaultEditBuilder.move and adds more test coverage.

Breaking Changes

None

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026 •
edited
Loading

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (212 lines, 3 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

yann-achard-MS
yann-achard-MS commented Jun 2, 2026
View reviewed changes
Comment thread packages/dds/tree/src/test/shared-tree/editing.spec.ts
expectJsonTree(tree, expectedState);
});

it("can move a node out from a field and into a field under a sibling", () => {
Copy link
Copy Markdown
Contributor Author

@yann-achard-MS yann-achard-MS Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario is now covered in src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts.

@yann-achard-MS yann-achard-MS marked this pull request as ready for review June 2, 2026 18:38
@yann-achard-MS yann-achard-MS requested a review from a team as a code owner June 2, 2026 18:38
Copilot AI review requested due to automatic review settings June 2, 2026 18:38
@yann-achard-MS yann-achard-MS changed the title refactor(tree): cleanup move logic refactor(tree): clarify move logic Jun 2, 2026
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

jason-ha
jason-ha approved these changes Jun 3, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - additional flow/clarity suggestions

Comment thread packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts Outdated
Comment thread packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts Outdated
Comment thread packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts
Comment thread packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts
jason-ha
jason-ha reviewed Jun 4, 2026
View reviewed changes
Comment thread packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts
jason-ha
jason-ha reviewed Jun 4, 2026
View reviewed changes
Comment thread packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts
jason-ha
jason-ha approved these changes Jun 5, 2026
View reviewed changes
Comment thread packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultChangeFamily.spec.ts
{ parent: root, field: fooKey },
1,
3,
{ parent: root_bar0, field: barKey },
Copy link
Copy Markdown
Contributor

@jason-ha jason-ha Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be nice to name the path: root.bar[0].bar, yes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jason-ha jason-ha jason-ha approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yann-achard-MS @jason-ha