chore: extract useCurrentAppState hook to reduce cyclic dependency#36937
chore: extract useCurrentAppState hook to reduce cyclic dependency#36937
Conversation
github-actions
bot
added
the
skip-changelog
|
Warning
|
| File Path | Change Summary |
|---|---|
app/client/src/pages/Editor/CanvasLayoutConversion/hooks/useShowSnapShotBanner.ts |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/IDE/Header/index.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/IDE/Layout/hooks/useGridLayoutTemplate.ts |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/IDE/Sidebar.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/WidgetsEditor/components/CodeModeTooltip.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/WidgetsEditor/components/NavigationAdjustedPageViewer.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/WidgetsEditor/components/NavigationPreview.tsx |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/WidgetsEditor/components/WidgetEditorNavigation.tsx |
Updated import path for useCurrentAppState. |
app/client/src/utils/hooks/useHoverToFocusWidget.ts |
Updated import path for useCurrentAppState. |
app/client/src/pages/Editor/IDE/hooks.ts |
Removed useCurrentAppState hook. |
app/client/src/pages/Editor/IDE/hooks/useCurrentAppState.ts |
Added useCurrentAppState hook managing application state. |
Possibly related PRs
- chore: Updating some states related to action execution #36801: This PR updates states related to action execution, which may involve similar hooks or context management as the changes made in the main PR regarding the
useCurrentAppStatehook.
Suggested labels
IDE Product, Task, ok-to-test, skip-changelog
Suggested reviewers
- ankitakinger
- ashit-rath
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?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
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)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/IDE/hooks/useCurrentAppState.ts (1)
6-9: Hook declaration and initial setup look good.The hook is well-structured and follows React conventions. Consider adding type annotations for better type safety:
const [appState, setAppState] = useState<EditorState>(EditorState.EDITOR); const entityInfo = identifyEntityFromPath(pathname);This will ensure that
appStateis always of typeEditorStateand make the code more robust.app/client/src/pages/Editor/WidgetsEditor/components/NavigationPreview.tsx (1)
Line range hint
1-46: Summary: Low-risk change with potential for improved dependency managementThe isolated nature of this change suggests a low risk of introducing new bugs. However, to ensure the effectiveness of this refactoring:
- Verify that this new import pattern is consistently applied across the codebase.
- Conduct thorough testing, particularly focusing on the functionality that depends on the
useCurrentAppStatehook.- Monitor for any unexpected behavior in the
NavigationPreviewcomponent during the review process.app/client/src/pages/Editor/IDE/Sidebar.tsx (1)
6-6: Approved: Import path update improves code structure.The change in the import path for
useCurrentAppStateis correct and aligns with the PR objective of extracting the hook to reduce cyclic dependencies. This modification should help improve the overall code structure.Consider using an absolute import path for consistency with other imports in this file:
-import { useCurrentAppState } from "./hooks/useCurrentAppState"; +import { useCurrentAppState } from "pages/Editor/IDE/hooks/useCurrentAppState";app/client/src/pages/Editor/IDE/Header/index.tsx (1)
Line range hint
1-73: Consider optimizing imports and component structure.While not directly related to the PR's main objective, consider the following suggestions to improve code maintainability:
- The
Headercomponent is quite large and complex. Consider splitting it into smaller, more manageable components.- There's a significant amount of logic within the component. Consider extracting some of this into custom hooks for better separation of concerns.
- The file has a large number of imports. Review if all of these are necessary or if some could be combined or removed.
These changes could improve code readability and make future maintenance easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- app/client/src/pages/Editor/CanvasLayoutConversion/hooks/useShowSnapShotBanner.ts (1 hunks)
- app/client/src/pages/Editor/IDE/Header/index.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/Layout/hooks/useGridLayoutTemplate.ts (1 hunks)
- app/client/src/pages/Editor/IDE/Sidebar.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/hooks.ts (0 hunks)
- app/client/src/pages/Editor/IDE/hooks/useCurrentAppState.ts (1 hunks)
- app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx (1 hunks)
- app/client/src/pages/Editor/WidgetsEditor/components/CodeModeTooltip.tsx (1 hunks)
- app/client/src/pages/Editor/WidgetsEditor/components/NavigationAdjustedPageViewer.tsx (1 hunks)
- app/client/src/pages/Editor/WidgetsEditor/components/NavigationPreview.tsx (1 hunks)
- app/client/src/pages/Editor/WidgetsEditor/components/WidgetEditorNavigation.tsx (1 hunks)
- app/client/src/utils/hooks/useHoverToFocusWidget.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/IDE/hooks.ts
🧰 Additional context used
🔇 Additional comments (13)
app/client/src/pages/Editor/IDE/hooks/useCurrentAppState.ts (4)
1-4: Imports look good.The necessary dependencies are correctly imported. The use of named imports enhances code readability.
11-13: Effect hook is implemented correctly.The
useEffecthook is used appropriately to update theappStatewhenentityInfo.appStatechanges. The dependency array is correctly specified, which helps in preventing unnecessary re-renders.
15-15: Return statement is concise and appropriate.The hook correctly returns the
appState, allowing components to access the current application state.
1-16: Overall, excellent implementation of theuseCurrentAppStatehook.This new file successfully extracts the
useCurrentAppStatehook, addressing the PR objective of reducing cyclic dependencies. The implementation is clean, follows React best practices, and effectively manages the application state based on the current route.A few points to highlight:
- The hook is well-structured and easy to understand.
- It correctly uses React hooks (
useState,useEffect) and react-router'suseLocation.- The code is concise and focused on a single responsibility.
This extraction should help in reducing the cyclic dependencies mentioned in the PR objectives. Great job!
app/client/src/pages/Editor/WidgetsEditor/components/CodeModeTooltip.tsx (1)
8-8: Approve import path update.The more specific import path aligns with the PR objective of extracting the
useCurrentAppStatehook. This change should help reduce cyclic dependencies as intended.Let's verify the new import path:
app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx (1)
5-5: LGTM. Import path update looks good.The change to the import path for
useCurrentAppStateis in line with the PR objective of extracting this hook. This should help reduce cyclic dependencies.Let's verify that this change is consistent across the codebase:
✅ Verification successful
Adjusting search commands to correctly target TypeScript files.
Let's re-run the verification with updated
rgcommands that specify file extensions directly:
Refining verification to specifically target
useCurrentAppStateimports.Let's re-run the verification with updated
rgcommands to ensureuseCurrentAppStateis no longer imported from the old path:
Import path update for
useCurrentAppStateverified successfully.All instances of
useCurrentAppStateare now imported from the updated path, and no imports remain from the old path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of useCurrentAppState from the old path # Test: Search for old import path. Expect: No results. rg --type typescript 'from "pages/Editor/IDE/hooks";\s*$' # Test: Search for new import path. Expect: Multiple results. rg --type typescript 'from "pages/Editor/IDE/hooks/useCurrentAppState"'Length of output: 203
Script:
# Description: Check for any remaining imports of useCurrentAppState from the old path # Test: Search for old import path in .ts files. Expect: No results. rg --glob "*.ts" 'from "pages/Editor/IDE/hooks";\s*$' # Test: Search for new import path in .ts files. Expect: Multiple results. rg --glob "*.ts" 'from "pages/Editor/IDE/hooks/useCurrentAppState"' # Test: Search for old import path in .tsx files. Expect: No results. rg --glob "*.tsx" 'from "pages/Editor/IDE/hooks";\s*$' # Test: Search for new import path in .tsx files. Expect: Multiple results. rg --glob "*.tsx" 'from "pages/Editor/IDE/hooks/useCurrentAppState"'Length of output: 1585
Script:
#!/bin/bash # Description: Ensure 'useCurrentAppState' is not imported from the old path # Test: Search for 'useCurrentAppState' imports from 'pages/Editor/IDE/hooks' in .ts files. Expect: No results. rg --glob "*.ts" 'import\s+\{\s*useCurrentAppState\s*\}\s+from\s+"pages/Editor/IDE/hooks";' # Test: Search for 'useCurrentAppState' imports from 'pages/Editor/IDE/hooks' in .tsx files. Expect: No results. rg --glob "*.tsx" 'import\s+\{\s*useCurrentAppState\s*\}\s+from\s+"pages/Editor/IDE/hooks";'Length of output: 188
app/client/src/pages/Editor/WidgetsEditor/components/NavigationPreview.tsx (1)
7-7: Approve: Import path update foruseCurrentAppStateThe change to a more specific import path aligns with the PR objective of reducing cyclic dependencies. This should help in improving the overall structure of the codebase.
To ensure the new import path is correct and consistent across the codebase, run the following script:
✅ Verification successful
Verified: Import path update for
useCurrentAppStateThe new import path has been correctly applied in
NavigationPreview.tsx, and no other instances ofuseCurrentAppStatewere found using the old path. This ensures consistency and aligns with the objective of reducing cyclic dependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new import path for useCurrentAppState rg --type typescript "import.*useCurrentAppState.*from" -g '!**/NavigationPreview.tsx'Length of output: 123
Script:
#!/bin/bash # Verify the new import path for useCurrentAppState by searching .ts and .tsx files rg "import\s+.*useCurrentAppState\s+from" --glob "*.ts" --glob "*.tsx" -g '!**/NavigationPreview.tsx'Length of output: 103
app/client/src/utils/hooks/useHoverToFocusWidget.ts (1)
8-8: Approved: Import path update looks good.The change to a more specific import path for
useCurrentAppStatealigns with the PR's objective of reducing cyclic dependencies.Let's verify if this change is consistent across the codebase:
app/client/src/pages/Editor/WidgetsEditor/components/WidgetEditorNavigation.tsx (1)
4-4: Approved: Import path update foruseCurrentAppStateThe change to import
useCurrentAppStatefrom a more specific path aligns with the PR's objective of extracting the hook to reduce cyclic dependencies. This modification should help in improving the overall structure of the codebase.To ensure the new import path is correct and consistent across the project, please run the following verification script:
✅ Verification successful
Verified: Import path for
useCurrentAppStateis consistentAll instances of
useCurrentAppStateare correctly imported from the updated path"pages/Editor/IDE/hooks/useCurrentAppState", ensuring consistency and alignment with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path for useCurrentAppState # Test: Search for all imports of useCurrentAppState rg --type typescript --type javascript 'import.*useCurrentAppState.*from' -g '!**/node_modules/**'Length of output: 135
Script:
#!/bin/bash # Description: Verify the import path for useCurrentAppState # Test: Search for all imports of useCurrentAppState with corrected file types rg --type ts --type js 'import.*useCurrentAppState.*from' -g '!**/node_modules/**'Length of output: 1523
app/client/src/pages/Editor/WidgetsEditor/components/NavigationAdjustedPageViewer.tsx (1)
Line range hint
1-93: LGTM: Component functionality remains intact.The
NavigationAdjustedPageViewercomponent's logic and usage ofuseCurrentAppStateremain unchanged. The import path update doesn't impact the component's functionality.app/client/src/pages/Editor/IDE/Layout/hooks/useGridLayoutTemplate.ts (1)
5-6: LGTM. Verify import consistency across the codebase.The import change for
useCurrentAppStatelooks good and aligns with the PR objective of extracting the hook to reduce cyclic dependencies.To ensure consistency, let's verify that this import change has been applied across the codebase:
✅ Verification successful
Resend Verification Scripts with Corrected Commands
The initial shell scripts failed to recognize the
typescriptfile type. Please execute the following updated scripts to verify import consistency across the codebase:
Import Consistency Verified
All imports of
useCurrentAppStatehave been successfully updated to the new location.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of useCurrentAppState from the old location # Test: Search for old import pattern. Expect: No results. rg --type typescript 'from "../../hooks";\s*$' -g '!**/useCurrentAppState.ts' # Test: Search for new import pattern. Expect: Multiple results. rg --type typescript 'from "../../hooks/useCurrentAppState";'Length of output: 213
Script:
#!/bin/bash # Description: Check for any remaining imports of useCurrentAppState from the old location # Test: Search for old import pattern in .ts and .tsx files. Expect: No results. rg 'from "../../hooks";\s*$' -g '*.ts' -g '*.tsx' # Test: Search for new import pattern in .ts and .tsx files. Expect: Multiple results. rg 'from "../../hooks/useCurrentAppState";' -g '*.ts' -g '*.tsx'Length of output: 1007
app/client/src/pages/Editor/IDE/Header/index.tsx (2)
73-73: Import path updated correctly.The import path for
useCurrentAppStatehas been updated from a relative path to a more specific one. This change aligns with the PR objective of extracting the hook to reduce cyclic dependencies.
Line range hint
1-391: Summary: PR objective achieved, with suggestions for future improvements.The main change of updating the import path for
useCurrentAppStatehas been implemented correctly, aligning with the PR's objective of reducing cyclic dependencies. The usage of the hook within the component is consistent with this change.While there are suggestions for potential improvements to the overall structure and organization of the component, these are not directly related to the PR's main goal. These suggestions could be considered for future refactoring efforts to enhance the maintainability of the codebase.
| import { EditorState } from "ee/entities/IDE/constants"; | ||
| import { getReadableSnapShotDetails } from "layoutSystems/autolayout/utils/AutoLayoutUtils"; | ||
| import { useCurrentAppState } from "pages/Editor/IDE/hooks"; | ||
| import { useCurrentAppState } from "pages/Editor/IDE/hooks/useCurrentAppState"; |
There was a problem hiding this comment.
💡 Codebase verification
Old import paths for hooks still present in some files.
app/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx:import { useGetPageFocusUrl } from "pages/Editor/IDE/hooks";app/client/src/pages/Editor/IDE/MainPane/index.tsx:import { useWidgetSelectionBlockListener } from "pages/Editor/IDE/hooks";
🔗 Analysis chain
Approve import path update.
The more specific import path for useCurrentAppState aligns with the PR objective of reducing cyclic dependencies. This change should help in isolating the hook and preventing unnecessary imports.
To ensure the new import path is correct and consistent across the codebase, run the following script:
</run_scripts>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the new import path for useCurrentAppState
# Search for any remaining old import paths
echo "Checking for any remaining old import paths:"
rg --type typescript 'from "pages/Editor/IDE/hooks";'
# Verify the new import path is used consistently
echo "Verifying consistent usage of the new import path:"
rg --type typescript 'from "pages/Editor/IDE/hooks/useCurrentAppState";'
Length of output: 410
Script:
#!/bin/bash
# Verify the new import path for useCurrentAppState
# Search for any remaining old import paths in .ts and .tsx files
echo "Checking for any remaining old import paths in TypeScript files:"
rg 'from "pages/Editor/IDE/hooks";' --glob '*.ts' --glob '*.tsx'
# Verify the new import path is used consistently in .ts and .tsx files
echo "Verifying consistent usage of the new import path in TypeScript files:"
rg 'from "pages/Editor/IDE/hooks/useCurrentAppState";' --glob '*.ts' --glob '*.tsx'
Length of output: 1774
| import React from "react"; | ||
| import { EditorState } from "ee/entities/IDE/constants"; | ||
| import { useCurrentAppState } from "pages/Editor/IDE/hooks"; | ||
| import { useCurrentAppState } from "pages/Editor/IDE/hooks/useCurrentAppState"; |
There was a problem hiding this comment.
💡 Codebase verification
Update Required: Inconsistent import paths for useCurrentAppState.
The following files still import useCurrentAppState from the old path "pages/Editor/IDE/hooks":
app/client/src/pages/Editor/IDE/MainPane/index.tsxapp/client/src/pages/Editor/IDE/EditorPane/components/PageElement.tsx
Please update these imports to "pages/Editor/IDE/hooks/useCurrentAppState" to ensure consistency across the codebase.
🔗 Analysis chain
LGTM: Import path update looks good.
The more specific import path for useCurrentAppState is a positive change. It potentially allows for better tree-shaking and clearer module organization.
Let's verify if this change is consistent across the codebase:
Let's adjust the search commands to correctly target TypeScript files and verify the import path consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of useCurrentAppState from the old path
# Test: Search for old import path. Expect: No results.
rg --type typescript 'from "pages/Editor/IDE/hooks";\s*$' -g '!**/NavigationAdjustedPageViewer.tsx'
# Test: Search for new import path. Expect: Multiple results.
rg --type typescript 'from "pages/Editor/IDE/hooks/useCurrentAppState";'
Length of output: 246
Script:
#!/bin/bash
# Description: Check for any remaining imports of useCurrentAppState from the old path
# Test: Search for old import path. Expect: No results.
rg 'from "pages/Editor/IDE/hooks";\s*$' -g "*.ts" -g "*.tsx" -g '!**/NavigationAdjustedPageViewer.tsx'
# Test: Search for new import path. Expect: Multiple results.
rg 'from "pages/Editor/IDE/hooks/useCurrentAppState";' -g "*.ts" -g "*.tsx"
Length of output: 1514
…ppsmithorg#36937) ## Description Extracts an IDE hook `useCurrentAppState` as it was used in multiple places. This usage was causing cyclic dependencies in various places as it was included in a file which was imported as well multiple times. This one extraction removes more than 100 cycle dependencies. ## Automation /ok-to-test tags="@tag.All" ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
2 participants
Description
Extracts an IDE hook
useCurrentAppStateas it was used in multiple places. This usage was causing cyclic dependencies in various places as it was included in a file which was imported as well multiple times. This one extraction removes more than 100 cycle dependencies.Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11382867731
Commit: 360bfb3
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Sanity/Datasources/RestApiOAuth2Validation_spec.ts
List of identified flaky tests.Thu, 17 Oct 2024 12:35:19 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?