Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(devtools-core): Render ST's Visualizer to Include Root Field's Allowed Types #23573

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

jikim-msft
Copy link
Contributor

@jikim-msft jikim-msft commented Jan 16, 2025

Description

26472

The current visualization generation logic in DefaultVisualizers.ts treats the root of the shared tree as a node and thus omits information that should be included in the root of the ST visualizer. Since the root field is allowed to have multiple types (i.e., different schema or primitive types), this information should be rendered in the tooltip of the visualizer.

This PR changes how the visualizer renders the allowed types in the tooltip

Example Schema

const config = new TreeViewConfiguration({
			schema: [RootNodeOne, RootNodeTwo, builder.string, builder.number],
		});

Before
Screenshot 2025-01-15 at 16 38 24

After
Defined
Screenshot 2025-01-29 at 19 48 01

Undefined
Screenshot 2025-01-29 at 19 43 32

@github-actions github-actions bot added the base: main PRs targeted against main branch label Jan 16, 2025
@jikim-msft jikim-msft requested a review from Josmithr January 17, 2025 23:36
@jikim-msft jikim-msft marked this pull request as ready for review January 25, 2025 02:14
@jikim-msft jikim-msft requested a review from a team as a code owner January 25, 2025 02:14
if (treeView === undefined) {
throw new Error("Support for visualizing empty trees is not implemented");
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is quite right. If the tree root is optional and there is no data (i.e., the root field is set to undefined), we will get undefined here. But there is still schema information to display.

This case should be like any other optional field with no data (undefined) that we encounter while walking the tree. So, I would expect us to just be able to call visualizeSharedTreeBySchema in this case like any other.

Copy link
Contributor

@Josmithr Josmithr Jan 29, 2025

Choose a reason for hiding this comment

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

Have we tested other cases where we have optional fields in the tree and no data is set? I'm curious how we display them currently. Ideally, we should still be able to see the allowed types for the field, even when there is no data in it.

Copy link
Contributor Author

@jikim-msft jikim-msft Jan 30, 2025

Choose a reason for hiding this comment

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

You're right. We should still be able to display the relevant schema information even if the tree root is optional (this isn't the case right now. I will fix it).

In addition, I personally think the non-root optional fields with no data should be omitted in the visualizer. Using the schema example in FluidObject.ts,

class TodoItem extends builder.object("todo-item", {
	title: builder.string,
	completed: builder.boolean,
	dueDate: builder.optional(builder.string),
	assignee: builder.optional(builder.string),
	colloborators: builder.optional(builder.array(builder.string)),
}) {}

class TodoObject extends builder.object("todo-list", {
	items: builder.array(TodoItem),
	optionalItem: builder.optional(builder.string), // CHANGE 1: Others are identical
}) {}

class TodoCategory extends builder.object("todo-category", {
	work: TodoObject,
	personal: TodoObject,
}) {}

class TodoWorkspace extends builder.object("todo-workspace", {
	lists: TodoCategory,
}) {}

const config = new TreeViewConfiguration({
	schema: [TodoWorkspace],
});

const view = sharedTree.viewWith(config);
view.initialize({
	lists: {
		work: {
			items: [
				{
					title: "Finish design doc.",
					completed: false,
					dueDate: "2048-01-01",
					assignee: "Kevin",
					colloborators: ["Rick"],
				},
				{
					title: "Review pull requests",
					completed: true,
					assignee: "Bob",
				},
			],
			optionalItem: "optional", // CHANGE 2: Others are identical
		},
		personal: {
			items: [
				{
					title: "Buy groceries",
					completed: false,
				},
				{
					title: "Schedule dentist appointment",
					completed: false,
					dueDate: "2024-05-04",
				},
			],
		},
	},
});

Screenshot 2025-01-29 at 17 25 38

I think the visualization above provides enough information to developers:

  • It gives the name of its direct child schema (todo-list). And I think this name is indicative enough for the devs to figure out that field optionalItem is indeed optional in the actual code.
  • It intentionally omits the unprovided data

With that said, I believe what we can do is:

  • Keep the current if condition and instead return the FluidObjectTreeNode withtooltipContents populated (which contains the information on optionality and direct child schema name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do something like this:

	const allowedTypes = concatenateTypes(sharedTree.exportSimpleSchema().allowedTypes);
	const isRequired = getIsRequired(sharedTree.exportSimpleSchema());

	if (treeView === undefined) {
		return {
			fluidObjectId: sharedTree.id,
			typeMetadata: "SharedTree",
			nodeKind: VisualNodeKind.FluidTreeNode,
			tooltipContents: {
				schema: {
					nodeKind: VisualNodeKind.TreeNode,
					children: {
						name: {
							nodeKind: VisualNodeKind.ValueNode,
							value: "undefined tree", // TODO: Better name
						},
						allowedTypes: {
							nodeKind: VisualNodeKind.ValueNode,
							value: allowedTypes,
						},
						isRequired: {
							nodeKind: VisualNodeKind.ValueNode,
							value: isRequired,
						},
					},
				},
			},
			children: {},
		};
	}

Screenshot 2025-01-29 at 18 41 09

Copy link
Contributor

Choose a reason for hiding this comment

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

If a field's value is undefined, I think we want to display that fact (i.e., that the data is undefined. But we should omit the schema field for it, since there is no data, so there isn't a corresponding schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I would probably make schema an optional property, and omit it for undefined field values.

*/
function storeObjectAllowedTypes(schema: SimpleObjectNodeSchema): {
allowedTypes: Record<string, string>;
requirements: Record<string, string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Were we going to change this from requirements: required / optional / identifier to required: true / false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. With the removal of the getRequirement(), I am simply filtering based on the Required & Optional kinds.

Is the name requirements misleading?

): Promise<VisualSharedTreeNode> {
const schemaFactory = new SchemaFactory(undefined);

return Tree.is(tree, [
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking: is there an existing "isLeaf" (or something) helper we could use for this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will double check on this. Thanks for the reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants