Canvas #1635

nmsimons · 2025-09-24T23:33:42Z

Add a new example that includes extensive use of presence and SharedTree.

jason-ha

Quick notes from brief look

jason-ha · 2025-10-01T16:53:14Z

canvas/src/presence/Interfaces/InkManager.ts

nit: consistency: Most files have header comments before imports

jason-ha · 2025-10-01T16:53:33Z

canvas/src/presence/Interfaces/SelectionManager.ts

nit: consistency: Most files have header comments before imports

jason-ha · 2025-10-01T17:02:48Z

canvas/src/presence/Interfaces/PresenceManager.ts

+	LatestRaw,
+	LatestRawEvents,
+	LatestMapRaw,
+	LatestMapRawEvents,


We should probably use the validated version of API in our sample.
Probably TypeBox is good solution. Presence readme has example.

If not, then we should comment why this example isn't even though validated pattern is our recommendation.

tylerbutler · 2025-10-01T17:06:26Z

canvas/build.yml

dupe? Doesn't seem to be in the right place.

tylerbutler · 2025-10-01T17:08:53Z

canvas/src/index.css

@@ -0,0 +1,198 @@
+@import "tailwindcss";
+
+/*


Do you need the commented out styles?

tylerbutler · 2025-10-01T17:12:40Z

canvas/src/presence/selection.ts

+	 * Implementation of the SelectionManager interface.
+	 * Handles multi-select operations and real-time state synchronization.
+	 */
+	class SelectionManagerImpl implements SelectionManager<Selection> {


I think you can reduce duplication between this and typedSelectionManager using a generics pattern like:

class SelectionManagerImpl<T extends Selection> implements SelectionManager<T> { // Single implementation for both types }

tylerbutler · 2025-10-01T17:14:14Z

canvas/src/react/components/app/App.tsx

+	// Initialize iOS Safari z-index fixes
+	useEffect(() => {
+		// Temporarily disabled to debug toolbar visibility
+		// fixIOSZIndexIssues();
+	}, []);


Still needed?

tylerbutler · 2025-10-01T17:15:32Z

canvas/src/start/azureStart.ts

+		// Commented out to suppress FluidDevToolsLogger console messages
+		// if (process.env.NODE_ENV === "development") {
+		// 	const { createDevtoolsLogger } = await import("@fluidframework/devtools/beta");
+		// 	logger = createDevtoolsLogger();
+		// }


Still needed?

tylerbutler · 2025-10-01T17:16:36Z

canvas/.editorconfig

Don't think we use these any more.

tylerbutler · 2025-10-01T17:18:16Z

canvas/EVENT_SUBSCRIPTION_CONSOLIDATION.md

This looks like a dev doc - maybe move this and similar ones into a docs folder or delete? I think they'll be confusing to people reading the example.

tylerbutler · 2025-10-01T17:19:36Z

canvas/src/infra/azure/azureTokenProvider.ts

+
+import { AzureMember, ITokenProvider, ITokenResponse, IUser } from "@fluidframework/azure-client";
+import { ScopeType } from "@fluidframework/protocol-definitions";
+import axios from "axios";


It still bugs me that we use axios instead of fetch, but probably not in scope for now.

WillieHabi · 2025-10-06T19:46:50Z

canvas/src/presence/Interfaces/UsersManager.ts

+	/** The user's profile information (name, avatar, etc.) */
+	value: TUserInfo;
+	/** The Fluid Framework client/attendee associated with this user */
+	client: Attendee;


Think we should be specific with attendee naming rather than client.

WillieHabi · 2025-10-06T19:49:59Z

canvas/src/presence/cursor.ts

+				) {
+					visibleCursors.push({
+						state: cursorState,
+						clientId: remote.attendee.attendeeId,


clientId is a seperate concept to attendeeId. Should probably replace clientId usage with attendeeId in CursorManager interface

WillieHabi · 2025-10-06T19:53:58Z

canvas/src/react/components/app/App.tsx

+// import "../../../styles/ios-fixes.css";
+// import "../../../styles/ios-safari-fixes.css";
+// import { fixIOSZIndexIssues } from "../../../utils/iosZIndexFix.js";


Can remove these commented out imports

WillieHabi · 2025-10-06T20:01:57Z

canvas/src/react/components/items/NoteView.tsx

+// A react component for displaying and interacting with notes using the Fluid Framework
+// Note object


Feel like this comment can go

WillieHabi · 2025-10-06T20:37:40Z

canvas/src/react/components/items/TableView.tsx

+// Custom sorting function for DateTime objects because
+// the default alphanumeric sorting function does not work
+// because the data is accessed via a second layer of the object
+const voteSortingFn: SortingFn<FluidRow> = (
+	rowA: Row<FluidRow>,
+	rowB: Row<FluidRow>,
+	columnId: string
+) => {
+	const valueA = rowA.getValue(columnId) as { value: Vote | undefined };
+	const valueB = rowB.getValue(columnId) as { value: Vote | undefined };
+	if (valueA === undefined && valueB === undefined) {
+		return 0;
+	} else if (valueA === undefined) {
+		return 1;
+	} else if (valueB === undefined) {
+		return -1;
+	} else if (Tree.is(valueA, Vote) && Tree.is(valueB, Vote)) {
+		const dateA = valueA.numberOfVotes;
+		const dateB = valueB.numberOfVotes;
+		if (dateA < dateB) {
+			return -1;
+		} else if (dateA > dateB) {
+			return 1;
+		} else {
+			return 0;
+		}
+	}
+	return 0;
+};


Comment says it sorting DateTime objects and we use dateA/dateB naming but this is a function for sorting votes

WillieHabi · 2025-10-06T20:42:13Z

canvas/src/react/components/items/TableView.tsx

+	const valueA = rowA.getValue(columnId) as { value: Vote | undefined };
+	const valueB = rowB.getValue(columnId) as { value: Vote | undefined };


Seeing inconsistencies between voteSortingFn and dateSortingFn. We cast valueA/valueB as nested { value: TValue | undefined } but then we do valueA.numberOfVotes.

We also check for nested valueA.value for undefined in dateSortingFn but not for voteSortingFn.

In any case these seem like the same function besides the nested value being compared. Can have a generic sorting function that takes in a compareFn to not repeat code.

EDIT: Tried adding votes column in the demo app and ran into some issues. Potential bug there.

WillieHabi · 2025-10-06T21:06:48Z

canvas/src/react/components/items/TableView.tsx

+// Get the sorting function and sort direction for a column
+const getSortingConfig = (
+	column: FluidColumn // Column object with id, name, and hint properties
+): { fn: SortingFnOption<FluidRow> | undefined; desc: boolean } => {
+	if (column.props.hint === hintValues.boolean) {
+		return { fn: "basic", desc: false };
+	} else if (column.props.hint === hintValues.number) {
+		return { fn: "alphanumeric", desc: true };
+	} else if (column.props.hint === hintValues.string) {
+		return { fn: "alphanumeric", desc: false };
+	} else if (column.props.hint === hintValues.date) {
+		return { fn: dateSortingFn, desc: false };
+	} else if (column.props.hint === hintValues.vote) {
+		return { fn: voteSortingFn, desc: true };
+	} else {
+		console.error("Unknown column type", "Hint:", column.props.hint);
+		return { fn: "basic", desc: false };
+	}
+};


nit: record/map or switch case over if else chain

WillieHabi · 2025-10-06T21:27:49Z

canvas/src/react/utils/dragUtils.ts

+export function getActiveDragForItem(
+	presence: React.ContextType<typeof PresenceContext>,
+	itemId: string
+): { id: string; x: number; y: number; rotation: number } | null {
+	const local = presence.drag?.state?.local as {
+		id: string;
+		x: number;
+		y: number;
+		rotation: number;
+	} | null;
+	if (local && local.id === itemId) return local;
+	const remotesIter = (
+		presence.drag?.state as unknown as { getRemotes?: () => unknown }
+	)?.getRemotes?.();
+	const isIterable = (obj: unknown): obj is Iterable<unknown> => {
+		return !!obj && typeof (obj as { [k: symbol]: unknown })[Symbol.iterator] === "function";
+	};
+	const isRecord = (v: unknown): v is Record<string, unknown> =>
+		typeof v === "object" && v !== null;
+	if (isIterable(remotesIter)) {


Lots of weird optional chaining and typecasts/typeguards here in this function. Is this a problem with Presence API? Seems like we should have more certainty around types used here.

nmsimons added 2 commits September 24, 2025 12:19

Add canvas sample

f83a1d3

build

17c6ccb

nmsimons requested a review from a team as a code owner September 24, 2025 23:33

jason-ha reviewed Oct 1, 2025

View reviewed changes

jason-ha requested a review from tylerbutler October 1, 2025 17:05

tylerbutler reviewed Oct 1, 2025

View reviewed changes

canvas/build.yml

Copy link

Member

tylerbutler Oct 1, 2025

Choose a reason for hiding this comment

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

dupe? Doesn't seem to be in the right place.

tylerbutler reviewed Oct 1, 2025

View reviewed changes

canvas/src/index.css

@@ -0,0 +1,198 @@

@import "tailwindcss";

/*

Copy link

Member

tylerbutler Oct 1, 2025

Choose a reason for hiding this comment

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

Do you need the commented out styles?

tylerbutler reviewed Oct 1, 2025

View reviewed changes

canvas/.editorconfig

Copy link

Member

tylerbutler Oct 1, 2025

Choose a reason for hiding this comment

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

Don't think we use these any more.

tylerbutler reviewed Oct 1, 2025

View reviewed changes

WillieHabi self-requested a review October 6, 2025 21:14

WillieHabi reviewed Oct 6, 2025

View reviewed changes

		// A react component for displaying and interacting with notes using the Fluid Framework
		// Note object

		const valueA = rowA.getValue(columnId) as { value: Vote \| undefined };
		const valueB = rowB.getValue(columnId) as { value: Vote \| undefined };

Canvas #1635

Are you sure you want to change the base?

Canvas #1635

Conversation

nmsimons commented Sep 24, 2025

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillieHabi Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillieHabi Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WillieHabi Oct 6, 2025 •

edited

Loading

WillieHabi Oct 6, 2025 •

edited

Loading