Skip to content

Commit

Permalink
Fix/remove navigator derived state (#6107)
Browse files Browse the repository at this point in the history
**Problem**
Currently, we recalculate `getNavigatorTargets` twice per frame, even
during canvas interactions where we know we won't use the results for
rendering the frame.

**Spike discovery**
What if we remove getNavigatorTargets from DerivedState and instead turn
it into a memoized selector?

**Details**
* create selector `navigatorTargetsSelector`, and try to use it from
every component that accessed derivedState.navigatorTargets or
derivedState.navigatorRows or derivedState.visibleNavigatorTargets
* create helper `getNavigatorTargetsFromEditorState` for all the Action
handler functions so they can keep using navigator targets without it
being in the DerivedState
* fix _a ton_ of test code so it uses
`getNavigatorTargetsFromEditorState`. this is the majority of the diff

**Evaluation**
I think the approach is very promising! Especially for frames during
canvas interactions where we want to disable most of the editor chrome,
the selectors give us a very clean way to _completely avoid_
calculations that are not directly necessary for updating the canvas. I
would like to continue to kill the rest of derived state in the same
vein. I believe I am over the halfway point, because navigatorTargets
was the majority of uses in our codebase.
  • Loading branch information
balazsbajorics authored Jul 23, 2024
1 parent 7e555e9 commit 05efe98
Show file tree
Hide file tree
Showing 38 changed files with 1,212 additions and 712 deletions.
31 changes: 26 additions & 5 deletions editor/src/components/canvas/canvas-context-menu.spec.browser2.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
import { MetadataUtils } from '../../core/model/element-metadata-utils'
import type { ElementPath } from '../../core/shared/project-file-types'
import { getDomRectCenter } from '../../core/shared/dom-utils'
import { getNavigatorTargetsFromEditorState } from '../navigator/navigator-utils'

function expectAllSelectedViewsToHaveMetadata(editor: EditorRenderResult) {
const selectedViews = editor.getEditorState().editor.selectedViews
Expand Down Expand Up @@ -814,7 +815,11 @@ describe('canvas context menu', () => {
'await-first-dom-report',
)

expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-utopia-storyboard-uid/scene-aaa',
'regular-utopia-storyboard-uid/scene-aaa/app-entity',
'regular-utopia-storyboard-uid/scene-aaa/app-entity:container',
Expand All @@ -831,7 +836,11 @@ describe('canvas context menu', () => {

await pressKey(']', { modifiers: cmdModifier }) // Bring Forward

expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-utopia-storyboard-uid/scene-aaa',
'regular-utopia-storyboard-uid/scene-aaa/app-entity',
'regular-utopia-storyboard-uid/scene-aaa/app-entity:container',
Expand All @@ -843,7 +852,11 @@ describe('canvas context menu', () => {

await pressKey('[', { modifiers: cmdModifier }) // Send Backward

expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-utopia-storyboard-uid/scene-aaa',
'regular-utopia-storyboard-uid/scene-aaa/app-entity',
'regular-utopia-storyboard-uid/scene-aaa/app-entity:container',
Expand All @@ -855,7 +868,11 @@ describe('canvas context menu', () => {

await pressKey(']', { modifiers: altCmdModifier }) // Bring To Front

expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-utopia-storyboard-uid/scene-aaa',
'regular-utopia-storyboard-uid/scene-aaa/app-entity',
'regular-utopia-storyboard-uid/scene-aaa/app-entity:container',
Expand All @@ -867,7 +884,11 @@ describe('canvas context menu', () => {

await pressKey('[', { modifiers: altCmdModifier }) // Send To Back

expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-utopia-storyboard-uid/scene-aaa',
'regular-utopia-storyboard-uid/scene-aaa/app-entity',
'regular-utopia-storyboard-uid/scene-aaa/app-entity:container',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import {
wait,
} from '../../../../utils/utils.test-utils'
import CanvasActions from '../../canvas-actions'
import { getNavigatorTargetsFromEditorState } from '../../../navigator/navigator-utils'

interface CheckCursor {
cursor: CSSCursor | null
Expand Down Expand Up @@ -905,7 +906,11 @@ export var ${BakedInStoryboardVariableName} = (props) => {
await mouseDoubleClickAtPoint(canvasControlsLayer, dragHereCenter)

// check that `drag-here` is expanded in the navigator
expect(editor.getEditorState().derived.navigatorTargets.map(navigatorEntryToKey)).toEqual([
expect(
getNavigatorTargetsFromEditorState(editor.getEditorState().editor).navigatorTargets.map(
navigatorEntryToKey,
),
).toEqual([
'regular-sb/scene1',
'regular-sb/scene1/container1',
'regular-sb/scene1/container1:container-root-div',
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { mouseClickAtPoint, mouseDragFromPointWithDelta } from '../../event-help
import { selectComponentsForTest } from '../../../../utils/utils.test-utils'
import * as EP from '../../../../core/shared/element-path'
import { navigatorEntryToKey } from '../../../../components/editor/store/editor-state'
import { getNavigatorTargetsFromEditorState } from '../../../navigator/navigator-utils'

async function dragElement(
renderResult: EditorRenderResult,
Expand Down Expand Up @@ -403,9 +404,9 @@ describe('Flex Reparent To Flex Strategy', () => {

await renderResult.getDispatchFollowUpActionsFinished()

const navigatorTargets = renderResult
.getEditorState()
.derived.visibleNavigatorTargets.map(navigatorEntryToKey)
const navigatorTargets = getNavigatorTargetsFromEditorState(
renderResult.getEditorState().editor,
).visibleNavigatorTargets.map(navigatorEntryToKey)
expect(navigatorTargets).toEqual([
'regular-utopia-storyboard-uid/parent2',
'regular-utopia-storyboard-uid/parent2/0b5',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import * as EP from '../../../../core/shared/element-path'
import { MetadataUtils } from '../../../../core/model/element-metadata-utils'
import { assert } from 'chai'
import { navigatorEntryToKey } from '../../../../components/editor/store/editor-state'
import { getNavigatorTargetsFromEditorState } from '../../../navigator/navigator-utils'

const TestProjectBlockElements = (additionalContainerStyle: string = '') => `
<div style={{ width: '100%', height: '100%', position: 'absolute', ${additionalContainerStyle} }} data-uid='container'>
Expand Down Expand Up @@ -316,7 +317,9 @@ async function dragElement(
modifiers: modifiers,
midDragCallback: async () => {
expect(
renderResult.getEditorState().derived.visibleNavigatorTargets.map(navigatorEntryToKey),
getNavigatorTargetsFromEditorState(
renderResult.getEditorState().editor,
).visibleNavigatorTargets.map(navigatorEntryToKey),
).toEqual(expectedNavigatorTargetsDuringMove)
},
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as EP from '../../../../core/shared/element-path'
import { getNavigatorTargetsFromEditorState } from '../../../navigator/navigator-utils'
import type { EditorRenderResult } from '../../ui-jsx.test-utils'
import type { FragmentLikeType } from './fragment-like-helpers'

Expand Down Expand Up @@ -57,9 +58,8 @@ export function getClosingFragmentLikeTag(type: FragmentLikeType): string {
}

export function getRegularNavigatorTargets(renderResult: EditorRenderResult): Array<string> {
return renderResult
.getEditorState()
.derived.navigatorTargets.filter((t) => t.type === 'REGULAR')
return getNavigatorTargetsFromEditorState(renderResult.getEditorState().editor)
.navigatorTargets.filter((t) => t.type === 'REGULAR')
.map((t) => t.elementPath)
.map(EP.toString)
}
Loading

0 comments on commit 05efe98

Please sign in to comment.