perf: clean up header routes & replace useLocation #37145
Conversation
WalkthroughThe pull request introduces significant changes to the routing structure within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/src/ce/pages/common/AppHeader.tsx (1)
Line range hint
1-17: Consider optimizing importsThe route constants could be destructured directly in the path array declarations to reduce the number of unused variables in the module scope.
import { - VIEWER_PATH, - BUILDER_PATH, - BUILDER_PATH_DEPRECATED, - VIEWER_PATH_DEPRECATED, - ADMIN_SETTINGS_CATEGORY_PATH, - VIEWER_CUSTOM_PATH, - BUILDER_CUSTOM_PATH, - BASE_URL, + BUILDER_PATH_DEPRECATED, BUILDER_PATH, BUILDER_CUSTOM_PATH, + VIEWER_PATH_DEPRECATED, VIEWER_PATH, VIEWER_CUSTOM_PATH, + ADMIN_SETTINGS_CATEGORY_PATH, BASE_URL, } from "constants/routes";app/client/src/pages/Editor/IDE/Header/index.tsx (1)
Line range hint
93-120: Consider memoizing HeaderTitleComponent for additional render optimization.While the current implementation is correct, memoizing this component could further reduce unnecessary re-renders when parent components update.
-const HeaderTitleComponent = ({ appState, currentPage }: HeaderTitleProps) => { +const HeaderTitleComponent = React.memo(({ appState, currentPage }: HeaderTitleProps) => { const libraryHeaderTitle = useLibraryHeaderTitle(); switch (appState) { case EditorState.DATA: return ( <IDEHeaderTitle key={appState} title={createMessage(HEADER_TITLES.DATA)} /> ); case EditorState.EDITOR: return <EditorTitle key={appState} title={currentPage?.pageName || ""} />; case EditorState.SETTINGS: return ( <IDEHeaderTitle key={appState} title={createMessage(HEADER_TITLES.SETTINGS)} /> ); case EditorState.LIBRARIES: return <IDEHeaderTitle key={appState} title={libraryHeaderTitle} />; default: return <EditorTitle key={appState} title={currentPage?.pageName || ""} />; } -}; +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/ce/pages/common/AppHeader.tsx(1 hunks)app/client/src/ee/pages/common/AppHeader.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(2 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(2 hunks)
🔇 Additional comments (7)
app/client/src/ee/pages/common/AppHeader.tsx (2)
1-4: Clean refactoring that improves performance!
The simplification from a class-based component to direct route exports aligns well with the goal of reducing unnecessary renders.
1-2: Verify CE routes coverage for EE features
Let's ensure the CE routes implementation handles all enterprise-specific routes.
✅ Verification successful
Let me check the EE-specific route implementations to ensure complete coverage.
Let me try a different approach to verify the routes.
CE routes implementation is sufficient for EE features
The EE AppHeader is a direct re-export of CE AppHeader with no additional routes or modifications. The CE implementation already covers all necessary routing paths including IDE, navigation, and page header routes. No EE-specific routes need to be handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any EE-specific routes that might need handling
# Look for route definitions in EE-specific components
rg -l "Route|useNavigate|useLocation" "app/client/src/ee"
# Compare with CE routes implementation
ast-grep --pattern 'const $_ = [$$$]' "app/client/src/ce/pages/common/AppHeader.tsx"
Length of output: 1229
Script:
#!/bin/bash
# Check EE-specific route definitions and compare with CE
rg -A 5 "export const.*Routes" app/client/src/ee/constants/routes/appRoutes.ts
# Check if there are any EE-specific route usages
rg "useNavigate|useLocation" app/client/src/ee/pages/common/AppHeader.tsx
# Look for any route-related constants or paths in EE AppHeader
ast-grep --pattern 'const $_ = [$$$]' app/client/src/ee/pages/common/AppHeader.tsx
Length of output: 241
Script:
#!/bin/bash
# Check the actual content of both CE and EE AppHeader files
cat app/client/src/ce/pages/common/AppHeader.tsx
echo "---EE AppHeader---"
cat app/client/src/ee/pages/common/AppHeader.tsx
# Look for any route-related imports or re-exports
rg -A 3 "import.*Route|export.*Route" app/client/src/ee/pages/common/AppHeader.tsx
Length of output: 1443
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (1)
11-13: LGTM: Effective use of useCallback for render optimization.
The memoization of closeMenu function aligns well with the PR's performance optimization goals.
app/client/src/ce/pages/common/AppHeader.tsx (2)
18-30: Well-structured path organization
The grouping of related paths into separate constants improves code organization and should help reduce unnecessary re-renders by consolidating route checks.
32-46: Clean route implementation, verify matching behavior
The consolidated routing approach looks good and aligns with the goal of reducing re-renders. However, let's verify that the route matching behaves correctly for all paths.
app/client/src/pages/Editor/IDE/Header/index.tsx (2)
80-80: LGTM: Import change aligns with performance objectives.
The addition of identifyEntityFromPath supports the transition from hook-based state management to a more efficient path-based approach.
158-158: Verify the path-based state determination impact.
The change from useCurrentAppState hook to identifyEntityFromPath should reduce unnecessary re-renders. However, let's verify all possible path patterns are handled correctly.
✅ Verification successful
Let me gather more information about the identifyEntityFromPath implementation since it wasn't found in the previous search.
Let me check the implementation of matchEntityFromPath since it's used within identifyEntityFromPath.
Let me check the implementation of getMatchPaths and getIDETypeByUrl to understand the path matching logic.
Let me check the EntityPaths and IDEBasePaths to complete the verification of path patterns.
Path-based state determination is correctly implemented
The change from useCurrentAppState hook to direct identifyEntityFromPath is safe. The implementation:
- Handles all IDE paths (editor, data, settings, libraries) via
EntityPaths - Supports both new and deprecated URL patterns through
IDEBasePaths - Maintains consistent state mapping across the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all possible path patterns that could affect appState determination
# Test: Search for route definitions and path patterns
rg -g '*.{ts,tsx}' -A 3 'EditorState\.'
# Test: Search for identifyEntityFromPath usage to ensure consistent implementation
ast-grep --pattern 'identifyEntityFromPath($path)'
Length of output: 26686
Script:
#!/bin/bash
# Search for identifyEntityFromPath implementation
rg -g '*.{ts,tsx}' -A 10 'function identifyEntityFromPath|const identifyEntityFromPath'
# Search for any imports of identifyEntityFromPath
rg -g '*.{ts,tsx}' 'import.*identifyEntityFromPath'
Length of output: 3526
Script:
#!/bin/bash
# Search for matchEntityFromPath implementation and its path patterns
rg -g '*.{ts,tsx}' -A 15 'function matchEntityFromPath|const matchEntityFromPath'
# Search for route patterns that might be used in matching
rg -g '*.{ts,tsx}' 'path:.*editor' -A 2
Length of output: 1161
Script:
#!/bin/bash
# Search for getMatchPaths and getIDETypeByUrl implementations
rg -g '*.{ts,tsx}' -A 10 'function getMatchPaths|const getMatchPaths|function getIDETypeByUrl|const getIDETypeByUrl'
# Search for IDE route patterns
rg -g '*.{ts,tsx}' 'const IDE_PATH|const BUILDER_PATH|const VIEWER_PATH' -A 3
Length of output: 3253
Script:
#!/bin/bash
# Search for EntityPaths and IDEBasePaths definitions
rg -g '*.{ts,tsx}' -A 5 'const EntityPaths|const IDEBasePaths'
# Search for any route path constants that might be used in these paths
rg -g '*.{ts,tsx}' 'const.*PATH.*=.*/(datasource|queries|api|jsObjects|settings|libraries)' -A 1
Length of output: 3880
alex-golovanov
added
ok-to-test
alex-golovanov
force-pushed
the
perf/37135-header-rerenders
branch
from
f8323b7
to
e52477f
Compare
alex-golovanov
added
ok-to-test
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Reduce the amount of unnecessary renders for app header.
Fixes #37135
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11667806569
Commit: dc0b5eb
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ServerSide/QueryPane/Querypane_Mongo_Spec.js
List of identified flaky tests.Tue, 05 Nov 2024 07:14:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
EditorTitlecomponent through optimization techniques.Bug Fixes
Headercomponent by memoizing it, preventing unnecessary updates.Refactor
Headercomponent.