Skip to content

Commit

Permalink
Follow based on connection id, disallow chained follows (#4762)
Browse files Browse the repository at this point in the history
  • Loading branch information
ruggi authored Jan 18, 2024
1 parent 82013b6 commit 041d49b
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 50 deletions.
4 changes: 3 additions & 1 deletion editor/src/components/canvas/multiplayer-presence.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,9 @@ const FollowingOverlay = React.memo(() => {

const isFollowTarget = React.useCallback(
(other: User<Presence, UserMeta>): boolean => {
return isFollowMode(mode) && other.id === mode.playerId
return (
isFollowMode(mode) && other.id === mode.playerId && other.connectionId === mode.connectionId
)
},
[mode],
)
Expand Down
4 changes: 3 additions & 1 deletion editor/src/components/editor/editor-modes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ export interface LiveCanvasMode {
export interface FollowMode {
type: 'follow'
playerId: string // the ID of the followed player
connectionId: number // the connection ID of the followed player
}

export type Mode =
Expand Down Expand Up @@ -248,10 +249,11 @@ export const EditorModes = {
isDragging: isDragging,
}
},
followMode: function (playerId: string): FollowMode {
followMode: function (playerId: string, connectionId: number): FollowMode {
return {
type: 'follow',
playerId: playerId,
connectionId: connectionId,
}
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3295,9 +3295,11 @@ export const CommentModeKeepDeepEquality: KeepDeepEqualityCall<CommentMode> = co
EditorModes.commentMode,
)

export const FollowModeKeepDeepEquality: KeepDeepEqualityCall<FollowMode> = combine1EqualityCall(
export const FollowModeKeepDeepEquality: KeepDeepEqualityCall<FollowMode> = combine2EqualityCalls(
(mode) => mode.playerId,
StringKeepDeepEquality,
(mode) => mode.connectionId,
NumberKeepDeepEquality,
EditorModes.followMode,
)

Expand Down
27 changes: 17 additions & 10 deletions editor/src/components/user-bar.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
/** @jsxRuntime classic */
/** @jsx jsx */
import React from 'react'
import { css, jsx } from '@emotion/react'
import { jsx } from '@emotion/react'
import type { CSSProperties } from 'react'
import { useOthers, useStatus, useStorage } from '../../liveblocks.config'
import { getUserPicture, isLoggedIn } from '../common/user'
import { getCollaborator, useMyUserAndPresence } from '../core/commenting/comment-hooks'
import type { MultiplayerColor } from '../core/shared/multiplayer'
import type { FollowTarget, MultiplayerColor } from '../core/shared/multiplayer'
import {
canFollowTarget,
followTarget,
isDefaultAuth0AvatarURL,
multiplayerColorFromIndex,
multiplayerInitialsFromName,
normalizeMultiplayerName,
normalizeOthersList,
} from '../core/shared/multiplayer'
import { MultiplayerWrapper } from '../utils/multiplayer-wrapper'
import { unless, when } from '../utils/react-conditionals'
Expand Down Expand Up @@ -133,6 +133,7 @@ const MultiplayerUserBar = React.memo(() => {
return {
...getCollaborator(collabs, other),
following: other.presence.following,
connectionId: other.connectionId,
}
}),
)
Expand Down Expand Up @@ -165,9 +166,13 @@ const MultiplayerUserBar = React.memo(() => {
}, [ownerId, myUser])

const toggleFollowing = React.useCallback(
(targetId: string) => () => {
(target: FollowTarget) => () => {
let actions: EditorAction[] = []
const canFollow = canFollowTarget(myUser.id, targetId, others)
const canFollow = canFollowTarget(
followTarget(myUser.id, myPresence.connectionId),
target,
others,
)
if (!canFollow) {
actions.push(
showToast(
Expand All @@ -181,14 +186,16 @@ const MultiplayerUserBar = React.memo(() => {
)
} else {
const newMode =
isFollowMode(mode) && mode.playerId === targetId
isFollowMode(mode) &&
mode.playerId === target.playerId &&
mode.connectionId === target.connectionId
? EditorModes.selectMode(null, false, 'none')
: EditorModes.followMode(targetId)
: EditorModes.followMode(target.playerId, target.connectionId)
actions.push(switchEditorMode(newMode))
}
dispatch(actions)
},
[dispatch, mode, myUser, others],
[dispatch, mode, others, myUser, myPresence],
)

if (myUser.name == null) {
Expand Down Expand Up @@ -218,8 +225,8 @@ const MultiplayerUserBar = React.memo(() => {
tooltip={{ text: name, colored: true }}
color={multiplayerColorFromIndex(other.colorIndex)}
picture={other.avatar}
onClick={toggleFollowing(other.id)}
isBeingFollowed={isFollowMode(mode) && mode.playerId === other.id}
onClick={toggleFollowing(followTarget(other.id, other.connectionId))}
isBeingFollowed={isFollowMode(mode) && mode.connectionId === other.connectionId}
follower={other.following === myUser.id}
isOwner={isOwner}
/>
Expand Down
56 changes: 39 additions & 17 deletions editor/src/core/shared/multiplayer.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { canFollowTarget, multiplayerInitialsFromName } from './multiplayer'
import { canFollowTarget, followTarget, multiplayerInitialsFromName } from './multiplayer'

describe('multiplayer', () => {
describe('multiplayerInitialsFromName', () => {
Expand All @@ -21,34 +21,56 @@ describe('multiplayer', () => {

describe('canFollowTarget', () => {
it('can follow a single player', () => {
expect(canFollowTarget('foo', 'bar', [{ id: 'bar', following: null }])).toBe(true)
})
it('can follow a player that follows another player', () => {
expect(
canFollowTarget('foo', 'bar', [
{ id: 'bar', following: 'baz' },
{ id: 'baz', following: null },
canFollowTarget(followTarget('foo', 0), followTarget('bar', 0), [
{ id: 'bar', connectionId: 0, following: null },
]),
).toBe(true)
})
it('can follow a player that follows another player indirectly', () => {
it('cannot follow self', () => {
expect(
canFollowTarget(followTarget('foo', 0), followTarget('foo', 0), [
{ id: 'foo', connectionId: 1, following: null },
]),
).toBe(false)
})
it('can follow a single player with the same ID but on another connection', () => {
expect(
canFollowTarget('foo', 'bar', [
{ id: 'bar', following: 'baz' },
{ id: 'baz', following: 'qux' },
{ id: 'qux', following: null },
canFollowTarget(followTarget('foo', 0), followTarget('foo', 1), [
{ id: 'foo', connectionId: 1, following: null },
]),
).toBe(true)
})
it('cannot follow a player that follows another player', () => {
expect(
canFollowTarget(followTarget('foo', 0), followTarget('bar', 0), [
{ id: 'bar', connectionId: 0, following: 'baz' },
{ id: 'baz', connectionId: 0, following: null },
]),
).toBe(false)
})
it('cannot follow a player that follows another player indirectly', () => {
expect(
canFollowTarget(followTarget('foo', 0), followTarget('bar', 0), [
{ id: 'bar', connectionId: 0, following: 'baz' },
{ id: 'baz', connectionId: 0, following: 'qux' },
{ id: 'qux', connectionId: 0, following: null },
]),
).toBe(false)
})
it('cannot follow a player back', () => {
expect(canFollowTarget('foo', 'bar', [{ id: 'bar', following: 'foo' }])).toBe(false)
expect(
canFollowTarget(followTarget('foo', 0), followTarget('bar', 0), [
{ id: 'bar', connectionId: 0, following: 'foo' },
]),
).toBe(false)
})
it('cannot follow a player that has an indirect loop', () => {
expect(
canFollowTarget('foo', 'bar', [
{ id: 'bar', following: 'baz' },
{ id: 'baz', following: 'qux' },
{ id: 'qux', following: 'foo' },
canFollowTarget(followTarget('foo', 0), followTarget('bar', 0), [
{ id: 'bar', connectionId: 0, following: 'baz' },
{ id: 'baz', connectionId: 0, following: 'qux' },
{ id: 'qux', connectionId: 0, following: 'foo' },
]),
).toBe(false)
})
Expand Down
44 changes: 24 additions & 20 deletions editor/src/core/shared/multiplayer.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
import { LiveObject, type CommentData, type ThreadData, type User } from '@liveblocks/client'
import {
useMutation,
useStorage,
type Presence,
type ThreadMetadata,
type UserMeta,
useStorage,
} from '../../../liveblocks.config'
import { possiblyUniqueInArray, safeIndex, stripNulls, uniqBy } from './array-utils'
import { colorTheme } from '../../uuiui'
import type { ElementPath } from './project-file-types'
import {
setHighlightedView,
switchEditorMode,
} from '../../components/editor/actions/action-creators'
import { EditorModes, existingComment } from '../../components/editor/editor-modes'
import { colorTheme } from '../../uuiui'
import { possiblyUniqueInArray, safeIndex, stripNulls, uniqBy } from './array-utils'
import { useMyUserId } from './multiplayer-hooks'
import type { ElementPath } from './project-file-types'

export type MultiplayerColor = {
background: string
Expand Down Expand Up @@ -105,25 +105,29 @@ export function isDefaultAuth0AvatarURL(s: string | null): boolean {
)
}

export function canFollowTarget(
selfId: string,
targetId: string | null,
others: { id: string; following: string | null }[],
): boolean {
let followChain: Set<string> = new Set()

let id = targetId
while (id != null) {
if (followChain.has(id)) {
return false
}
followChain.add(id)
export type FollowTarget = {
playerId: string
connectionId: number
}

const target = others.find((o) => o.id === id)
id = target?.following ?? null
export function followTarget(playerId: string, connectionId: number): FollowTarget {
return {
playerId: playerId,
connectionId: connectionId,
}
}

return !followChain.has(selfId)
export function canFollowTarget(
from: FollowTarget,
to: FollowTarget,
others: { id: string; following: string | null; connectionId: number }[],
): boolean {
if (from.playerId === to.playerId && from.connectionId === to.connectionId) {
return false
}
return !others.some(
(o) => o.id === to.playerId && o.connectionId === to.connectionId && o.following != null,
)
}

const roomIdPrefix = `project-room-`
Expand Down

0 comments on commit 041d49b

Please sign in to comment.