Skip to content

Commit

Permalink
skip calling getRemixValidPathsGenerationContext if not in remixScene (
Browse files Browse the repository at this point in the history
…#6073)

**Problem:**
When resizing a PromiseCard, it takes 7-8 ms to run useGetStoryboardRoot
/ getValidElementPaths which seems to spend most of its time on Remix
matchRoutes.

**Diagnosis:**
Turns out, we had this three lines of code running for _every single
element visited_ during the recursive `getValidElementPathsFromElement`
walk:
```    
const isRemixScene = isRemixSceneElement(element, filePath, projectContents)
    const remixPathGenerationContext = getRemixValidPathsGenerationContext(path)
    if (remixPathGenerationContext.type === 'active' && isRemixScene) {
```
`getRemixValidPathsGenerationContext` is not trivially cheap to call,
and it only returns a sensible result if `path` points at a remix scene.

**Fix:**
Only call getRemixValidPathsGenerationContext if `isRemixScene` is true.

Now `useGetStoryboardRoot` only takes 0.7ms instead of 7ms.

<img width="120" alt="image"
src="https://github.com/user-attachments/assets/750c33ac-13e1-43fd-90d1-200439ab5708">

**Note:** 0.7ms still feels excessive to run on every single frame, I'll
continue the investigation and make a follow up PR

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #6065
  • Loading branch information
balazsbajorics authored and liady committed Dec 13, 2024
1 parent 1a36d3f commit df2b44e
Showing 1 changed file with 44 additions and 42 deletions.
86 changes: 44 additions & 42 deletions editor/src/components/canvas/canvas-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1774,55 +1774,57 @@ function getValidElementPathsFromElement(
let paths = includeElementInPath ? [path] : []
if (isJSXElementLike(element)) {
const isRemixScene = isRemixSceneElement(element, filePath, projectContents)
const remixPathGenerationContext = getRemixValidPathsGenerationContext(path)
if (remixPathGenerationContext.type === 'active' && isRemixScene) {
function makeValidPathsFromModule(routeModulePath: string, parentPathInner: ElementPath) {
const file = getProjectFileByFilePath(projectContents, routeModulePath)
if (file == null || file.type !== 'TEXT_FILE') {
return
}
if (isRemixScene) {
const remixPathGenerationContext = getRemixValidPathsGenerationContext(path)
if (remixPathGenerationContext.type === 'active') {
function makeValidPathsFromModule(routeModulePath: string, parentPathInner: ElementPath) {
const file = getProjectFileByFilePath(projectContents, routeModulePath)
if (file == null || file.type !== 'TEXT_FILE') {
return
}

const topLevelElement = getDefaultExportedTopLevelElement(file)
const topLevelElement = getDefaultExportedTopLevelElement(file)

if (topLevelElement == null) {
return
}
if (topLevelElement == null) {
return
}

paths.push(
...getValidElementPathsFromElement(
focusedElementPath,
topLevelElement,
parentPathInner,
projectContents,
autoFocusedPaths,
routeModulePath,
uiFilePath,
false,
true,
true,
resolve,
getRemixValidPathsGenerationContext,
),
)
}
paths.push(
...getValidElementPathsFromElement(
focusedElementPath,
topLevelElement,
parentPathInner,
projectContents,
autoFocusedPaths,
routeModulePath,
uiFilePath,
false,
true,
true,
resolve,
getRemixValidPathsGenerationContext,
),
)
}

/**
* The null check is here to guard against the case when a route with children is missing an outlet
* that would render the children
*/

for (const route of remixPathGenerationContext.currentlyRenderedRouteModules) {
const entry = remixPathGenerationContext.routeModulesToRelativePaths[route.id]
if (entry != null) {
const { relativePaths, filePath: filePathOfRouteModule } = entry
for (const relativePath of relativePaths) {
const basePath = EP.appendTwoPaths(path, relativePath)
makeValidPathsFromModule(filePathOfRouteModule, basePath)
/**
* The null check is here to guard against the case when a route with children is missing an outlet
* that would render the children
*/

for (const route of remixPathGenerationContext.currentlyRenderedRouteModules) {
const entry = remixPathGenerationContext.routeModulesToRelativePaths[route.id]
if (entry != null) {
const { relativePaths, filePath: filePathOfRouteModule } = entry
for (const relativePath of relativePaths) {
const basePath = EP.appendTwoPaths(path, relativePath)
makeValidPathsFromModule(filePathOfRouteModule, basePath)
}
}
}
}

return paths
return paths
}
}

const isScene = isSceneElement(element, filePath, projectContents)
Expand Down

0 comments on commit df2b44e

Please sign in to comment.