Revert "chore: Move IDE header to ADS/Templates (#37406)" #37739
Conversation
This reverts commit 8cb0bee.
WalkthroughThis pull request involves the removal of several files related to the Changes
Suggested labels
Suggested reviewers
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
|
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
app/client/src/IDE/Components/HeaderTitle.tsx (1)
Line range hint
4-15: Enhance JSDoc documentation.While the component implementation is clean, consider improving the JSDoc to document the
titleprop and its purpose./** - * Handy little styled component that can be used to render the title in the IDEHeader component + * A styled component that renders a title in the IDE header. + * @param {Object} props - Component props + * @param {string} props.title - The text to display as the title */app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (2)
8-8: Consider extracting the props interfaceFor better reusability and maintainability, consider extracting the props interface:
+interface EditorTitleProps { + title: string; +} -const EditorTitle = ({ title }: { title: string }) => { +const EditorTitle = ({ title }: EditorTitleProps) => {
9-9: Consider using useBoolean hookFor boolean state management, consider using the useBoolean hook for consistency with the codebase's patterns. Also, explicitly set the initial state value.
-const [active, setActive] = useState(false); +const [active, setActive] = useState<boolean>(false); // or preferably: +const [active, { setTrue: openMenu, setFalse: closeMenu }] = useBoolean(false);app/client/src/IDE/Components/HeaderDropdown.test.tsx (1)
6-21: Consider adding more comprehensive test coverage.While the basic rendering test is good, consider adding tests for:
- Empty header/body scenarios
- Multiple children in header/body
- Nested components
app/client/src/IDE/Components/HeaderDropdown.tsx (3)
6-8: Consider making grid columns more maintainableThe grid template with 8 columns (one being 0) seems overly specific and might be brittle to changes.
Consider using a more flexible approach:
- grid-template-columns: 0 auto 1fr auto auto auto auto auto; + grid-template-columns: auto 1fr repeat(5, auto);
28-39: Add JSDoc documentation for interfacesThe interfaces are well-structured, but adding JSDoc documentation would improve maintainability.
+/** Represents the main dropdown container component */ interface EditorHeaderDropdown { children: React.ReactNode; } +/** Represents the header section of the dropdown */ interface EditorHeaderDropdownHeader { children: React.ReactNode; className?: string; } +/** Represents the body section of the dropdown */ interface EditorHeaderDropdownBody { children: React.ReactNode; }
41-74: Consider making maxHeight configurableThe fixed maxHeight of 300px might not accommodate all use cases.
+interface EditorHeaderDropdown { + children: React.ReactNode; + maxHeight?: string; +} -function EditorHeaderDropdown({ children }: EditorHeaderDropdown) { +function EditorHeaderDropdown({ children, maxHeight = "300px" }: EditorHeaderDropdown) { return ( <Container flexDirection="column" justifyContent="center" - maxHeight="300px" + maxHeight={maxHeight} overflow="hidden" >app/client/src/IDE/Components/HeaderEditorSwitcher.tsx (3)
5-12: Add JSDoc documentation to the interface.Adding documentation will improve developer experience and code maintainability.
+/** + * Props for the HeaderEditorSwitcher component + * @property {string} prefix - The prefix text to display + * @property {string} [title] - Optional title text + * @property {string} titleTestId - Test ID for the title element + * @property {boolean} active - Whether the switcher is in active state + * @property {React.MouseEventHandler<HTMLDivElement>} [onClick] - Optional click handler + * @property {string} [className] - Optional CSS class name + */ interface HeaderEditorSwitcherProps { prefix: string; title?: string; titleTestId: string; active: boolean; onClick?: React.MouseEventHandler<HTMLDivElement>; className?: string; }
14-23: Enhance type safety for the styled component.Consider using a more specific type for the
activeprop.-const SwitchTrigger = styled.div<{ active: boolean }>` +type SwitchTriggerProps = { + active: boolean; +}; +const SwitchTrigger = styled.div<SwitchTriggerProps>`
36-36: Use a className utility for better class composition.Consider using a utility like
clsxorclassnamesfor more robust className handling.- className={`flex align-center items-center justify-center ${className}`} + className={clsx('flex align-center items-center justify-center', className)}app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (2)
Line range hint
44-53: Add missing dependency to useCallbackThe
instanceIdis used withincreatePageCallbackbut is missing from the dependency array. This could lead to stale closure issues.Apply this fix:
}, [dispatch, pages, applicationId]); + }, [dispatch, pages, applicationId, workspaceId, instanceId]);
66-81: Consider using template literals for className concatenationThe component structure looks good, but the className concatenation could be improved for better readability.
Consider updating the className prop:
- className={`${EntityClassNames.ADD_BUTTON} group pages`} + className={`${EntityClassNames.ADD_BUTTON} group pages`}app/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx (2)
16-26: Consider enhancing basic render test coverageWhile the current assertions check for text content, consider adding checks for:
- Component structure using
container.querySelector- Accessibility attributes
- Default styling classes
28-36: Enhance active state test coverageConsider adding assertions for:
- Other visual changes in active state
- Transition between active/inactive states
- Icon state changes if applicable
app/client/src/pages/Editor/IDE/Header/index.tsx (1)
95-95: Consider adding a meaningful fallback for empty page namesWhile the optional chaining is good, an empty string fallback might not provide the best user experience. Consider using a more meaningful default.
- return <EditorTitle key={appState} title={currentPage?.pageName || ""} />; + return <EditorTitle key={appState} title={currentPage?.pageName || "Untitled"} />;Also applies to: 107-107, 118-118
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (32)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/index.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.constants.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.mdx(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx(0 hunks)app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts(0 hunks)app/client/packages/design-system/ads/src/Templates/index.ts(0 hunks)app/client/packages/design-system/ads/src/index.ts(0 hunks)app/client/src/IDE/Components/HeaderDropdown.test.tsx(1 hunks)app/client/src/IDE/Components/HeaderDropdown.tsx(1 hunks)app/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx(2 hunks)app/client/src/IDE/Components/HeaderEditorSwitcher.tsx(1 hunks)app/client/src/IDE/Components/HeaderTitle.tsx(2 hunks)app/client/src/IDE/index.ts(2 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/AdminSettings/components.tsx(1 hunks)app/client/src/pages/AppViewer/Navigation/Sidebar.tsx(1 hunks)app/client/src/pages/Editor/EditorName/components.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx(3 hunks)app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(5 hunks)app/client/src/pages/Editor/JSEditor/styledComponents.ts(1 hunks)app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx(1 hunks)app/client/src/pages/Editor/commons/EditorHeaderComponents.tsx(1 hunks)app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx(1 hunks)app/client/src/pages/Editor/commons/Omnibar.tsx(1 hunks)app/client/src/pages/Editor/index.tsx(1 hunks)
💤 Files with no reviewable changes (13)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/ListWithHeader.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/index.ts
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/styles.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/HeaderSwitcher.styles.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/IDEHeaderSwitcher.tsx
- app/client/packages/design-system/ads/src/Templates/IDEHeader/HeaderSwitcher/index.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.constants.ts
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.mdx
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.stories.tsx
- app/client/packages/design-system/ads/src/Templates/IDEHeader/IDEHeader.tsx
- app/client/packages/design-system/ads/src/Templates/IDEHeader/index.ts
- app/client/packages/design-system/ads/src/Templates/index.ts
- app/client/packages/design-system/ads/src/index.ts
✅ Files skipped from review due to trivial changes (5)
- app/client/src/pages/Editor/WidgetsEditor/WidgetEditorContainer.tsx
- app/client/src/pages/Editor/commons/EditorHeaderComponents.tsx
- app/client/src/pages/Editor/commons/EditorWrapperContainer.tsx
- app/client/src/pages/Editor/commons/Omnibar.tsx
- app/client/src/pages/Editor/index.tsx
🔇 Additional comments (24)
app/client/src/IDE/Components/HeaderTitle.tsx (2)
1-2: LGTM! Clean imports.
The consolidated imports from @appsmith/ads improve code organization.
17-17: Verify import statements across the codebase.
The change from named export to default export requires updates to import statements in other files.
✅ Verification successful
Let me gather more information about the imports to understand the full context.
No changes needed for imports
The barrel file (IDE/index.ts) already exports HeaderTitle as a default export using export { default as IDEHeaderTitle }. The change to default export in HeaderTitle.tsx is consistent with the existing barrel file export and current usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files that might need import statement updates
# Search for old named import pattern
echo "Files potentially needing import updates:"
rg "import.*\{.*IDEHeaderTitle.*\}.*from.*HeaderTitle"
# Search for current usage to verify migration
echo -e "\nCurrent import usage:"
rg "import.*HeaderTitle.*from"
Length of output: 645
Script:
#!/bin/bash
# Check the actual content of the IDE barrel file and HeaderTitle imports
echo "Checking IDE barrel file exports:"
rg -A 5 "export.*HeaderTitle" "app/client/src/IDE/index.ts"
echo -e "\nChecking HeaderTitle component usage:"
ast-grep --pattern 'import { $$ } from "IDE"'
Length of output: 604
app/client/src/pages/Editor/IDE/Header/EditorTitle.tsx (1)
1-6: Verify the IDE import path resolution
The import path 'IDE' seems unusually short. This might cause issues with path resolution in different environments.
✅ Verification successful
Let me verify the IDEHeaderEditorSwitcher import by checking its actual location.
Let me try a different search pattern to locate the component.
Let me check the export in the IDE/index.ts file to confirm the component's availability.
The 'IDE' import path is correct and properly configured
The short import path 'IDE' is valid as the component IDEHeaderEditorSwitcher is properly exported from the IDE module's index file at app/client/src/IDE/index.ts. This is a deliberate architectural choice for clean imports from the IDE module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how the IDE path is resolved
fd -e ts -e tsx -e js -e jsx --full-path '.*IDE.*' | grep -v "node_modules"
Length of output: 9760
Script:
#!/bin/bash
# Check for IDEHeaderEditorSwitcher component
ast-grep --pattern 'export $_ IDEHeaderEditorSwitcher'
Length of output: 56
Script:
#!/bin/bash
# Search for HeaderEditorSwitcher in IDE directory
rg -l "HeaderEditorSwitcher" app/client/src/IDE/
Length of output: 191
Script:
#!/bin/bash
# Check the exports in IDE/index.ts
cat app/client/src/IDE/index.ts
Length of output: 3642
app/client/src/pages/Editor/EditorName/components.ts (2)
Line range hint 8-45: LGTM! Styled components implementation remains clean and consistent
The usage of IDE_HEADER_HEIGHT constant in the styled components is maintained correctly, providing consistent header sizing across components.
4-5: Verify the IDE_HEADER_HEIGHT constant from new location
The constant has been moved from @appsmith/ads to IDE module. Let's ensure the constant exists and maintains the same value to prevent layout issues.
✅ Verification successful
IDE_HEADER_HEIGHT constant is correctly moved and consistently used
The constant is properly defined in IDE/Structure/constants.ts with value 40 and all imports across the codebase consistently use the new "IDE" module path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify IDE_HEADER_HEIGHT constant exists and check its value
# Search for the constant definition in the new location
rg -p "export.*IDE_HEADER_HEIGHT.*=.*" --type ts
# Check all other usages to ensure consistent import path
rg -p "import.*IDE_HEADER_HEIGHT.*from.*" --type ts
Length of output: 1178
app/client/src/IDE/Components/HeaderDropdown.test.tsx (2)
1-4: LGTM! All necessary testing dependencies are properly imported.
1-40: Verify HeaderDropdown component dependencies after revert.
Since this is part of a revert PR, ensure that all necessary dependencies for HeaderDropdown are properly maintained.
✅ Verification successful
Let me search for any references to this component to verify its dependencies properly.
Let me search for any references and dependencies of the HeaderDropdown component to verify its usage in the codebase.
HeaderDropdown component is properly maintained in its new location
The component is correctly imported and used from its new location in IDE/Components. The only consumer is PagesSection.tsx which imports it through the IDE barrel file (index.ts). No remaining references to the old location were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to the old location
rg -l "ADS/Templates.*IDEHeader"
# Verify the new HeaderDropdown location references
rg -l "IDE/Components/HeaderDropdown"
Length of output: 72
Script:
#!/bin/bash
# Search for HeaderDropdown references in the codebase with proper path handling
rg -l "HeaderDropdown"
# Check for any IDE header related imports or components
rg -l "IDEHeader|HeaderDropdown" app/client/src/
# Look for component usage patterns
ast-grep --pattern 'import { $$ } from "$_/HeaderDropdown"'
ast-grep --pattern '<HeaderDropdown$$$>'
Length of output: 139128
Script:
#!/bin/bash
# Search for HeaderDropdown usage in the codebase
rg -l "HeaderDropdown" app/client/src/
# Look for any imports of HeaderDropdown
rg "import.*HeaderDropdown" app/client/src/
# Check for any IDE header related components
rg -l "IDEHeader" app/client/src/
Length of output: 794
app/client/src/pages/AdminSettings/components.tsx (2)
Line range hint 41-47: LGTM: LoaderContainer implementation is correct
The LoaderContainer's height calculation using IDE_HEADER_HEIGHT is properly maintained despite the import change.
3-4: Verify the IDE import path consistency
The import path change from "@appsmith/ads" to "IDE" needs verification across the codebase to ensure consistent usage.
✅ Verification successful
Import path for IDE_HEADER_HEIGHT is correctly aligned with codebase
The import change from "@appsmith/ads" to "IDE" is consistent with the codebase:
- All other files import IDE_HEADER_HEIGHT from "IDE" module
- The constant is properly exported from IDE/Structure/constants.ts
- No remaining references to the old @appsmith/ads import path exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the consistency of IDE_HEADER_HEIGHT imports across the codebase
# Search for all IDE_HEADER_HEIGHT imports
echo "Checking IDE_HEADER_HEIGHT import patterns:"
rg "import.*IDE_HEADER_HEIGHT.*from" --type ts --type tsx
# Search for any remaining references to the old import path
echo -e "\nChecking for any remaining old import paths:"
rg "import.*IDE_HEADER_HEIGHT.*from.*@appsmith/ads" --type ts --type tsx
Length of output: 392
Script:
#!/bin/bash
# Search for IDE_HEADER_HEIGHT imports with correct file extensions
echo "Checking IDE_HEADER_HEIGHT import patterns:"
rg "import.*IDE_HEADER_HEIGHT.*from" -g "*.ts" -g "*.tsx"
# Search for any remaining references to the old import path
echo -e "\nChecking for any remaining old import paths:"
rg "import.*IDE_HEADER_HEIGHT.*from.*@appsmith/ads" -g "*.ts" -g "*.tsx"
# Check if IDE_HEADER_HEIGHT is exported from IDE module
echo -e "\nChecking IDE_HEADER_HEIGHT exports in IDE module:"
rg "export.*IDE_HEADER_HEIGHT" -g "*.ts" -g "*.tsx"
Length of output: 1632
app/client/src/IDE/Components/HeaderDropdown.tsx (1)
76-79: LGTM! Clean compound component implementation
The component composition follows React best practices with a clear compound component pattern.
app/client/src/pages/Editor/JSEditor/styledComponents.ts (2)
Line range hint 35-42: Implementation looks good, pending import verification
The FormWrapper's height calculation using IDE_HEADER_HEIGHT is implemented correctly.
9-9: Verify the IDE module import path
The import path "IDE" seems unusually short and might cause issues. Typically, import paths should be more specific.
✅ Verification successful
The previous results show that IDE_HEADER_HEIGHT is defined in app/client/src/IDE/Structure/constants.ts, but we need to verify if it's properly exported from the IDE module and how other files are importing it. This will help determine if the import path is correct.
Import path "IDE" is correct and properly configured
The import path is valid as IDE_HEADER_HEIGHT is:
- Defined in
app/client/src/IDE/Structure/constants.ts - Correctly re-exported through
app/client/src/IDE/index.ts - Consistently imported as
import { IDE_HEADER_HEIGHT } from "IDE"across multiple files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the IDE module exists and contains IDE_HEADER_HEIGHT
# Check if the IDE module exists and find its location
fd -t f "IDE.(ts|tsx|js|jsx)$"
# Search for IDE_HEADER_HEIGHT export
ast-grep --pattern 'export const IDE_HEADER_HEIGHT'
Length of output: 272
Script:
#!/bin/bash
# Check imports of IDE_HEADER_HEIGHT across the codebase
rg "IDE_HEADER_HEIGHT" -B 2 -A 2
# Check if there's an index file in IDE directory
fd -t f "index.(ts|tsx|js|jsx)$" app/client/src/IDE/
# Check barrel exports in IDE directory
rg "export.*IDE_HEADER_HEIGHT" app/client/src/IDE/
Length of output: 10724
app/client/src/pages/Editor/IDE/EditorPane/PagesSection.tsx (2)
Line range hint 1-23: LGTM! Import statements are well-organized
The imports are properly structured and the new IDEHeaderDropdown import aligns with the component's requirements.
Line range hint 1-81: Verify migration of ListWithHeader usages
Let's ensure all usages of the old ListWithHeader component have been properly migrated to IDEHeaderDropdown.
✅ Verification successful
Let me search for any remaining ListWithHeader components to ensure complete migration.
Let me try another search to find any remaining ListWithHeader usages.
Migration from ListWithHeader to IDEHeaderDropdown appears complete
Based on the search results, there are no remaining instances of ListWithHeader in the codebase, and the new IDEHeaderDropdown component is properly implemented in PagesSection.tsx. The migration appears to be successful.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of ListWithHeader
rg "ListWithHeader"
# Search for new IDEHeaderDropdown usage patterns
rg "IDEHeaderDropdown"
Length of output: 925
Script:
#!/bin/bash
# Search for ListWithHeader component definition and imports
ast-grep --pattern 'import { $_ as ListWithHeader } from "$_"'
ast-grep --pattern 'import { ListWithHeader } from "$_"'
ast-grep --pattern 'const ListWithHeader = $_'
# Search for any ListWithHeader JSX usage
ast-grep --pattern '<ListWithHeader $$$>'
Length of output: 142142
Script:
#!/bin/bash
# Search for ListWithHeader in all files, including history
rg -l "ListWithHeader" --hidden
# Search for any file containing both ListWithHeader and IDEHeaderDropdown to find migration patterns
rg -l "ListWithHeader.*IDEHeaderDropdown|IDEHeaderDropdown.*ListWithHeader" --hidden
Length of output: 118
app/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx (3)
38-51: LGTM! Basic interaction tests are well covered
The click handler and ref forwarding tests adequately cover the basic functionality.
1-15: Verify the component import path after revert
Since this is a revert PR, let's ensure the import path for HeaderEditorSwitcher is correct and matches the reverted file structure.
✅ Verification successful
Import path for HeaderEditorSwitcher component is correctly configured
The verification confirms that both the component and its test file exist at the expected location in the codebase:
app/client/src/IDE/Components/HeaderEditorSwitcher.tsxapp/client/src/IDE/Components/HeaderEditorSwitcher.test.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the component exists at the specified path
fd -e tsx -e ts "HeaderEditorSwitcher" app/client/src/IDE/Components/
Length of output: 184
Line range hint 53-104: Verify test alignment with reverted component behavior
The test cases look comprehensive, but since this is a revert PR, please verify that:
- The icon color variables match the reverted design system
- The class names being tested align with the reverted component's implementation
✅ Verification successful
Tests are correctly aligned with the component implementation
The verification confirms that:
- The color variable
--ads-v2-colors-content-label-inactive-fgis consistently used in both the component and tests - The class names being tested (
flex,align-center,justify-center) match exactly with the classes applied in the component implementation - The test cases accurately reflect the component's behavior and styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related style definitions in the codebase
rg -t ts -t tsx "ads-v2-color" app/client/src/
rg -t ts -t tsx "ads-v2-colors-content-label-inactive-fg" app/client/src/
Length of output: 176
Script:
#!/bin/bash
# Let's try searching without file type restrictions since tsx wasn't recognized
rg "ads-v2-colors-content-label-inactive-fg" app/client/src/
# Search for the HeaderEditorSwitcher component implementation
fd HeaderEditorSwitcher.tsx app/client/src/
# Let's also check the component's styles
fd HeaderEditorSwitcher.styled.tsx app/client/src/
# Search for any design system color definitions
rg "colors-content-label" app/client/src/
Length of output: 1432
Script:
#!/bin/bash
# Let's check the component's class names in the implementation
rg "className=" app/client/src/IDE/Components/HeaderEditorSwitcher.tsx
# Also check for any styled components that might define these classes
rg "flex|align-center|justify-center" app/client/src/IDE/Components/HeaderEditorSwitcher.tsx -A 2 -B 2
Length of output: 452
app/client/src/IDE/index.ts (2)
6-14: LGTM! Documentation and exports are well-structured.
The documentation clearly explains the composable nature of the IDEHeader component.
Let's verify the consistent usage of IDE_HEADER_HEIGHT:
✅ Verification successful
IDE_HEADER_HEIGHT constant is consistently used across the codebase
The constant is properly exported and used consistently for header height calculations in layout components, with no hardcoded values found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded header height values that should use the constant instead
rg -g '*.{ts,tsx,css,scss}' -i 'height:.*?(?:56|64)px'
# Verify the constant usage
rg "IDE_HEADER_HEIGHT"
Length of output: 87742
28-43: Verify backward compatibility of the new header components.
The new header components (HeaderTitle, HeaderEditorSwitcher, HeaderDropdown) look well-structured, but since this is a revert PR, we should ensure they maintain compatibility with existing usage patterns.
Let's check for potential breaking changes:
✅ Verification successful
Let me gather more information about the component implementations to verify backward compatibility.
Components are well-structured with stable interfaces
The new header components maintain backward compatibility:
- HeaderEditorSwitcher has a stable interface with expected props (prefix, title, active state)
- HeaderDropdown follows the compound component pattern with Header/Body sections
- Components are being used correctly in the codebase with proper prop passing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of the old components that might be affected
rg -A 3 "IDEHeaderSwitcher|ListWithHeader"
# Check for any direct imports of the new components
rg -A 3 "import.*(?:HeaderTitle|HeaderEditorSwitcher|HeaderDropdown)"
Length of output: 2578
Script:
#!/bin/bash
# Check the implementation of HeaderEditorSwitcher to verify its interface
ast-grep --pattern 'interface $_ {
$$$
}' app/client/src/IDE/Components/HeaderEditorSwitcher.tsx
# Check the implementation of HeaderDropdown to verify its interface
ast-grep --pattern 'interface $_ {
$$$
}' app/client/src/IDE/Components/HeaderDropdown.tsx
# Check the implementation of HeaderTitle to verify its interface
ast-grep --pattern 'interface $_ {
$$$
}' app/client/src/IDE/Components/HeaderTitle.tsx
# Check actual usage patterns in the codebase
rg -A 5 "<(?:HeaderTitle|HeaderEditorSwitcher|HeaderDropdown)"
Length of output: 7651
app/client/src/pages/AppViewer/Navigation/Sidebar.tsx (1)
37-37: Verify the IDE_HEADER_HEIGHT import path
The import path change aligns with the revert operation. Let's verify the consistency of this import across the codebase.
✅ Verification successful
Let me try to verify the import path change by searching for both the imports and the constant definition. The previous command had an issue with file type recognition, so I've adjusted it to explicitly include TypeScript and TSX files.
Let me try searching without file type restrictions since there seems to be an issue with the typescript/tsx file type recognition. This should help us verify both the import path and the constant definition.
Import path change is correct and consistent
The constant IDE_HEADER_HEIGHT is properly defined in app/client/src/IDE/Structure/constants.ts and exported through IDE/index.ts. All imports across the codebase consistently use the "IDE" path, including the one in Sidebar.tsx. The import change aligns with the codebase's structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all IDE_HEADER_HEIGHT imports to ensure consistency
rg "import.*IDE_HEADER_HEIGHT.*from" --type ts --type tsx
# Search for the IDE_HEADER_HEIGHT constant definition
ast-grep --pattern 'export const IDE_HEADER_HEIGHT = $_'
Length of output: 142172
Script:
#!/bin/bash
# Search for IDE_HEADER_HEIGHT imports with ripgrep, including tsx files
rg -t typescript -t tsx "import.*IDE_HEADER_HEIGHT.*from"
# Search for IDE_HEADER_HEIGHT constant definition
rg -t typescript -t tsx "export const IDE_HEADER_HEIGHT"
Length of output: 188
Script:
#!/bin/bash
# Search for IDE_HEADER_HEIGHT imports without file type restrictions
rg "import.*IDE_HEADER_HEIGHT.*from"
# Search for IDE_HEADER_HEIGHT constant definition
rg "export const IDE_HEADER_HEIGHT"
# Find all files that might contain IDE_HEADER_HEIGHT
rg "IDE_HEADER_HEIGHT"
Length of output: 3648
app/client/src/pages/Editor/IDE/Header/index.tsx (3)
76-77: LGTM: Import changes are appropriate
The new imports align well with the interface changes and component restructuring.
92-92: LGTM: Interface extension is well-typed
The optional currentPage prop is properly typed as Page and maintains backward compatibility by being optional.
221-222: LGTM: Header structure properly updated
The new IDEHeader structure is correctly implemented with proper prop passing.
app/client/src/ce/constants/messages.ts (1)
2488-2488: Navigation title change looks good
The change from EDITOR to "Pages" in the header titles makes the navigation more intuitive for users.
✅ Verification successful
Navigation title change looks good
The change from "EDITOR" to "Pages" in the header titles is consistent with the codebase. The term "Pages" is more intuitive for users and aligns with the application's navigation structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no hardcoded references to old "EDITOR" header title remain
rg -i "EDITOR" --type ts --type tsx
Length of output: 63
Script:
#!/bin/bash
# Search for EDITOR references in TypeScript files
rg -i "EDITOR" -t ts
# Also check for any React component files that might have .tsx extension
fd -e tsx -x rg -i "EDITOR" {}
Length of output: 103169
| <PopoverContent | ||
| align="start" | ||
| className="!p-0 !pb-1" | ||
| onEscapeKeyDown={closeMenu} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using !important CSS flags
The use of !important flags (!p-0 !pb-1) suggests potential CSS specificity issues. Consider refactoring the styles to avoid using !important.
-className="!p-0 !pb-1"
+className="popover-content"Then define the styles with appropriate specificity in your CSS/SCSS file.
Committable suggestion skipped: line range outside the PR's diff.
| it("applies custom className to the header", () => { | ||
| const customClass = "my-custom-class"; | ||
| const { container } = render( | ||
| <HeaderDropdown> | ||
| <HeaderDropdown.Header className={customClass}> | ||
| <span>Header</span> | ||
| </HeaderDropdown.Header> | ||
| <HeaderDropdown.Body> | ||
| <span>Body</span> | ||
| </HeaderDropdown.Body> | ||
| </HeaderDropdown>, | ||
| ); | ||
|
|
||
| const headerElement = container.querySelector(`.${customClass}`); | ||
|
|
||
| expect(headerElement).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using RTL queries instead of querySelector.
Replace container.querySelector with RTL's getByTestId or getByRole for better testing practices. This makes tests more resilient to implementation changes.
- const headerElement = container.querySelector(`.${customClass}`);
+ const headerElement = getByTestId('header-dropdown');Add a data-testid attribute to your HeaderDropdown component:
<HeaderDropdown.Header className={customClass} data-testid="header-dropdown">| const HeaderEditorSwitcher = React.forwardRef( | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (props: HeaderEditorSwitcherProps, ref: any) => { |
There was a problem hiding this comment.
Fix the ref type instead of disabling eslint.
The TODO comment indicates a known issue with type safety. Let's fix it properly.
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (props: HeaderEditorSwitcherProps, ref: any) => {
+ (
+ props: HeaderEditorSwitcherProps,
+ ref: React.ForwardedRef<HTMLDivElement>
+ ) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const HeaderEditorSwitcher = React.forwardRef( | |
| // TODO: Fix this the next time the file is edited | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (props: HeaderEditorSwitcherProps, ref: any) => { | |
| const HeaderEditorSwitcher = React.forwardRef( | |
| ( | |
| props: HeaderEditorSwitcherProps, | |
| ref: React.ForwardedRef<HTMLDivElement> | |
| ) => { |
…ppsmithorg#37739) This reverts commit 8cb0bee. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced `HeaderEditorSwitcher` and `HeaderDropdown` components for enhanced UI interactions. - **Bug Fixes** - Updated import paths for `IDE_HEADER_HEIGHT` across multiple components to ensure consistent styling. - **Refactor** - Renamed and restructured components for clarity and improved functionality, including `HeaderTitle` and `IDEHeaderSwitcher`. - **Tests** - Added tests for the `HeaderDropdown` component to validate rendering and functionality. - **Chores** - Removed deprecated components and files to streamline the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This reverts commit 8cb0bee.
Summary by CodeRabbit
HeaderEditorSwitcherandHeaderDropdowncomponents for enhanced UI interactions.IDE_HEADER_HEIGHTacross multiple components to ensure consistent styling.HeaderTitleandIDEHeaderSwitcher.HeaderDropdowncomponent to validate rendering and functionality.