Skip to content

Commit 520128d

Browse files
authored
fix(editor): address low-severity manager audit findings (tldraw#8895)
In order to address the low-severity editor manager audit findings, this PR tightens several manager contracts without changing `Editor` accessors. It centralizes blur completion in `FocusManager`, reduces collaborator visibility work, lazily starts the collaborator visibility clock, names the small-screen edge-scroll tuning constants, and makes pointer velocity smoothing elapsed-aware. Relates to tldraw#8894. ### Change type - [x] `improvement` ### Test plan 1. `cd packages/editor && yarn test run` 2. `cd packages/tldraw && yarn test run src/test/Editor.test.tsx -t "Editor.TickManager"` 3. `yarn typecheck` 4. `yarn api-check` - [x] Unit tests - [ ] End to end tests ### Code changes | Section | LOC change | | --------------- | ---------- | | Core code | +67 / -77 | | Tests | +163 / -0 | | Config/tooling | +1 / -1 |
1 parent 673e94a commit 520128d

11 files changed

Lines changed: 231 additions & 78 deletions

File tree

.vscode/extensions.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
{
2-
"recommendations": ["esbenp.prettier-vscode"]
2+
"recommendations": ["oxc.oxc-vscode"]
33
}

packages/editor/src/lib/editor/Editor.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10372,11 +10372,7 @@ export class Editor extends EventEmitter<TLEventMap> {
1037210372
*/
1037310373
blur({ blurContainer = true } = {}): this {
1037410374
if (!this.getIsFocused()) return this
10375-
if (blurContainer) {
10376-
this.focusManager.blur()
10377-
} else {
10378-
this.complete() // stop any interaction
10379-
}
10375+
this.focusManager.blur({ blurContainer })
1038010376
this.updateInstanceState({ isFocused: false })
1038110377
return this
1038210378
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import { PageRecordType, type TLInstancePresence } from '@tldraw/tlschema'
2+
import { vi } from 'vitest'
3+
import type { Editor } from '../../Editor'
4+
import { CollaboratorsManager } from './CollaboratorsManager'
5+
6+
const currentPageId = PageRecordType.createId('page')
7+
8+
function createPresence(userId: string): TLInstancePresence {
9+
return {
10+
typeName: 'instance_presence',
11+
id: `instance_presence:${userId}` as TLInstancePresence['id'],
12+
userId,
13+
userName: userId,
14+
lastActivityTimestamp: Date.now(),
15+
color: '#000000',
16+
camera: null,
17+
selectedShapeIds: [],
18+
currentPageId,
19+
brush: null,
20+
scribbles: [],
21+
screenBounds: null,
22+
followingUserId: null,
23+
cursor: null,
24+
chatMessage: '',
25+
meta: {},
26+
}
27+
}
28+
29+
function createEditor(presences: TLInstancePresence[] = []) {
30+
const setInterval = vi.fn(() => 123)
31+
const getInstanceState = vi.fn(() => ({
32+
followingUserId: null,
33+
highlightedUserIds: [],
34+
}))
35+
const userGetId = vi.fn(() => 'current-user')
36+
37+
const editor = {
38+
options: {
39+
collaboratorCheckIntervalMs: 1000,
40+
collaboratorIdleTimeoutMs: 3000,
41+
collaboratorInactiveTimeoutMs: 5000,
42+
},
43+
timers: {
44+
setInterval,
45+
},
46+
user: {
47+
getId: userGetId,
48+
},
49+
store: {
50+
query: {
51+
records: vi.fn(() => ({
52+
get: () => presences,
53+
})),
54+
},
55+
},
56+
getInstanceState,
57+
getCurrentPageId: vi.fn(() => currentPageId),
58+
} as unknown as Editor
59+
60+
return { editor, setInterval, getInstanceState, userGetId }
61+
}
62+
63+
describe(CollaboratorsManager, () => {
64+
afterEach(() => {
65+
vi.clearAllMocks()
66+
})
67+
68+
it('starts the visibility clock on the first visible collaborators read', () => {
69+
const { editor, setInterval } = createEditor()
70+
const manager = new CollaboratorsManager(editor)
71+
72+
expect(setInterval).not.toHaveBeenCalled()
73+
74+
expect(manager.getVisibleCollaborators()).toEqual([])
75+
76+
expect(setInterval).toHaveBeenCalledTimes(1)
77+
expect(setInterval).toHaveBeenCalledWith(expect.any(Function), 1000)
78+
})
79+
80+
it('only starts the visibility clock once across repeated reads', () => {
81+
const { editor, setInterval } = createEditor()
82+
const manager = new CollaboratorsManager(editor)
83+
84+
manager.getVisibleCollaborators()
85+
manager.getVisibleCollaborators()
86+
manager.getVisibleCollaborators()
87+
88+
expect(setInterval).toHaveBeenCalledTimes(1)
89+
})
90+
91+
it('reads instance state once when filtering visible collaborators', () => {
92+
const { editor, getInstanceState } = createEditor([
93+
createPresence('user-1'),
94+
createPresence('user-2'),
95+
])
96+
const manager = new CollaboratorsManager(editor)
97+
98+
expect(manager.getVisibleCollaborators()).toHaveLength(2)
99+
100+
expect(getInstanceState).toHaveBeenCalledTimes(1)
101+
})
102+
103+
it('hides idle collaborators that are following us', () => {
104+
const presence = createPresence('peer')
105+
presence.lastActivityTimestamp = Date.now() - 4000
106+
presence.followingUserId = 'current-user'
107+
const { editor } = createEditor([presence])
108+
const manager = new CollaboratorsManager(editor)
109+
110+
expect(manager.getVisibleCollaborators()).toEqual([])
111+
})
112+
113+
it('shows idle collaborators that are following us when they have a chat message', () => {
114+
const presence = createPresence('peer')
115+
presence.lastActivityTimestamp = Date.now() - 4000
116+
presence.followingUserId = 'current-user'
117+
presence.chatMessage = 'hi'
118+
const { editor } = createEditor([presence])
119+
const manager = new CollaboratorsManager(editor)
120+
121+
expect(manager.getVisibleCollaborators()).toHaveLength(1)
122+
})
123+
124+
it('shows idle collaborators that are not following us', () => {
125+
const presence = createPresence('peer')
126+
presence.lastActivityTimestamp = Date.now() - 4000
127+
const { editor } = createEditor([presence])
128+
const manager = new CollaboratorsManager(editor)
129+
130+
expect(manager.getVisibleCollaborators()).toHaveLength(1)
131+
})
132+
})

packages/editor/src/lib/editor/managers/CollaboratorsManager/CollaboratorsManager.ts

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,6 @@
11
import { EMPTY_ARRAY, atom, computed } from '@tldraw/state'
2-
import { TLInstancePresence } from '@tldraw/tlschema'
2+
import type { TLInstancePresence } from '@tldraw/tlschema'
33
import { maxBy } from '@tldraw/utils'
4-
import {
5-
getCollaboratorStateFromElapsedTime,
6-
shouldShowCollaborator,
7-
} from '../../../utils/collaboratorState'
84
import type { Editor } from '../../Editor'
95

106
/**
@@ -17,12 +13,19 @@ import type { Editor } from '../../Editor'
1713
* @public
1814
*/
1915
export class CollaboratorsManager {
20-
constructor(private readonly editor: Editor) {
16+
constructor(private readonly editor: Editor) {}
17+
18+
private _visibilityClockStarted = false
19+
20+
private _startVisibilityClock() {
21+
if (this._visibilityClockStarted) return
22+
this._visibilityClockStarted = true
23+
2124
// Editor disposes `editor.timers` on its own teardown, so the interval is
2225
// automatically cleared when the editor is disposed.
23-
editor.timers.setInterval(() => {
26+
this.editor.timers.setInterval(() => {
2427
this._visibilityClock.set(Date.now())
25-
}, editor.options.collaboratorCheckIntervalMs)
28+
}, this.editor.options.collaboratorCheckIntervalMs)
2629
}
2730

2831
/**
@@ -75,14 +78,39 @@ export class CollaboratorsManager {
7578
*/
7679
@computed
7780
getVisibleCollaborators(): TLInstancePresence[] {
81+
const { editor } = this
82+
const { collaboratorInactiveTimeoutMs, collaboratorIdleTimeoutMs } = editor.options
83+
84+
this._startVisibilityClock()
7885
this._visibilityClock.get()
7986
const now = Date.now()
80-
return this.getCollaborators().filter((presence) => {
87+
const collaborators = this.getCollaborators()
88+
if (!collaborators.length) return EMPTY_ARRAY
89+
90+
const { followingUserId, highlightedUserIds } = this.editor.getInstanceState()
91+
const currentUserId = this.editor.user.getId()
92+
93+
return collaborators.filter((presence) => {
94+
const { lastActivityTimestamp, userId, chatMessage } = presence
95+
8196
// Treat a missing `lastActivityTimestamp` as "active right now" (elapsed = 0)
8297
// so newly-joined peers aren't immediately classified as idle/inactive.
83-
const elapsed = Math.max(0, now - (presence.lastActivityTimestamp ?? now))
84-
const state = getCollaboratorStateFromElapsedTime(this.editor, elapsed)
85-
return shouldShowCollaborator(this.editor, presence, state)
98+
const elapsed = Math.max(0, now - (lastActivityTimestamp ?? now))
99+
100+
if (elapsed > collaboratorInactiveTimeoutMs) {
101+
// Inactive: If they're inactive, only show if we're following them or they're highlighted
102+
return followingUserId === userId || highlightedUserIds.includes(userId)
103+
}
104+
105+
if (elapsed > collaboratorIdleTimeoutMs) {
106+
// Idle: If they're idle and following us, hide them unless they have a chat message or are highlighted
107+
if (presence.followingUserId === currentUserId) {
108+
return !!(chatMessage || highlightedUserIds.includes(userId))
109+
}
110+
}
111+
112+
// Active
113+
return true
86114
})
87115
}
88116

packages/editor/src/lib/editor/managers/EdgeScrollManager/EdgeScrollManager.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ import { EASINGS } from '../../../primitives/easings'
22
import { Vec } from '../../../primitives/Vec'
33
import type { Editor } from '../../Editor'
44

5+
// Tuned for touch/small-screen feel; adjust together with coarsePointerWidth.
6+
const EDGE_SCROLL_SMALL_SCREEN_THRESHOLD_PX = 1000
7+
const EDGE_SCROLL_SMALL_SCREEN_SPEED_FACTOR = 0.612
8+
59
/** @public */
610
export class EdgeScrollManager {
711
constructor(public editor: Editor) {}
@@ -115,8 +119,14 @@ export class EdgeScrollManager {
115119
const screenBounds = editor.getViewportScreenBounds()
116120

117121
// Determines how much the speed is affected by the screen size
118-
const screenSizeFactorX = screenBounds.w < 1000 ? 0.612 : 1
119-
const screenSizeFactorY = screenBounds.h < 1000 ? 0.612 : 1
122+
const screenSizeFactorX =
123+
screenBounds.w < EDGE_SCROLL_SMALL_SCREEN_THRESHOLD_PX
124+
? EDGE_SCROLL_SMALL_SCREEN_SPEED_FACTOR
125+
: 1
126+
const screenSizeFactorY =
127+
screenBounds.h < EDGE_SCROLL_SMALL_SCREEN_THRESHOLD_PX
128+
? EDGE_SCROLL_SMALL_SCREEN_SPEED_FACTOR
129+
: 1
120130

121131
// Determines the base speed of the scroll
122132
const zoomLevel = editor.getZoomLevel()

packages/editor/src/lib/editor/managers/FocusManager/FocusManager.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,13 @@ describe('FocusManager', () => {
334334

335335
expect(callOrder).toEqual(['complete', 'blur'])
336336
})
337+
338+
it('should complete without blurring the container when blurContainer is false', () => {
339+
focusManager.blur({ blurContainer: false })
340+
341+
expect(editor.complete).toHaveBeenCalled()
342+
expect(mockContainer.blur).not.toHaveBeenCalled()
343+
})
337344
})
338345

339346
describe('dispose', () => {

packages/editor/src/lib/editor/managers/FocusManager/FocusManager.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,9 @@ export class FocusManager {
8080
this.editor.getContainer().focus()
8181
}
8282

83-
blur() {
83+
blur({ blurContainer = true } = {}) {
8484
this.editor.complete() // stop any interaction
85-
this.editor.getContainer().blur() // blur the container
85+
if (blurContainer) this.editor.getContainer().blur() // blur the container
8686
}
8787

8888
dispose() {

packages/editor/src/lib/editor/managers/InputsManager/InputsManager.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ import { isAccelKey } from '../../../utils/keyboard'
77
import type { Editor } from '../../Editor'
88
import { TLPinchEventInfo, TLPointerEventInfo, TLWheelEventInfo } from '../../types/event-types'
99

10+
const POINTER_VELOCITY_REFERENCE_INTERVAL_MS = 16
11+
const POINTER_VELOCITY_REFERENCE_SMOOTHING = 0.5
12+
1013
/** @public */
1114
export class InputsManager {
1215
constructor(private readonly editor: Editor) {}
@@ -473,8 +476,14 @@ export class InputsManager {
473476
const length = delta.len()
474477
const direction = length ? delta.div(length) : new Vec(0, 0)
475478

476-
// consider adjusting this with an easing rather than a linear interpolation
477-
const next = pointerVelocity.clone().lrp(direction.mul(length / elapsed), 0.5)
479+
// Preserve the old 16ms smoothing with alpha = 1 - (1 - 0.5)^(elapsed / 16).
480+
const smoothing =
481+
1 -
482+
Math.pow(
483+
1 - POINTER_VELOCITY_REFERENCE_SMOOTHING,
484+
elapsed / POINTER_VELOCITY_REFERENCE_INTERVAL_MS
485+
)
486+
const next = pointerVelocity.clone().lrp(direction.mul(length / elapsed), smoothing)
478487

479488
// if the velocity is very small, just set it to 0
480489
if (Math.abs(next.x) < 0.01) next.x = 0

packages/editor/src/lib/hooks/usePeerIds.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { useEditor } from './useEditor'
44

55
/**
66
* Reactive list of peer user IDs for collaborators currently shown in the UI.
7+
* This list includes user ids who are not on the user's current page.
78
* Mirrors {@link Editor.getVisibleCollaborators} — peers fade out as they
89
* transition to idle/inactive, without requiring a manual re-render.
910
*

packages/editor/src/lib/utils/collaboratorState.ts

Lines changed: 0 additions & 54 deletions
This file was deleted.

0 commit comments

Comments
 (0)