feat: Overflow tabs list view #34150
Conversation
…o overflow-tabs-list-view
…o overflow-tabs-list-view
github-actions
bot
added
IDE Navigation
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9453109750. |
WalkthroughThe changes span several areas of the IDE's file tab management and rendering. Key updates include the addition of new components ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IDE
participant EditorPane
participant EditorState
User ->> IDE: New Tab Click
IDE ->> EditorPane: Render AddTab Component
EditorPane ->> EditorState: Create New Tab
EditorState -->> IDE: Updated Tab State
IDE ->> User: New Tab Rendered
User ->> IDE: Toggle Fullscreen
IDE ->> EditorPane: Trigger ScreenModeToggle Component
EditorPane ->> EditorState: Update ViewMode
EditorState -->> IDE: Updated ViewMode State
IDE ->> User: Fullscreen Mode Activated
Assessment against linked issues
Poem
Warning Review ran into problemsProblems (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2)
67-77: UseuseCallbackeffectively.The
useCallbackhook is used here to memoize thetoggleEditorModefunction. While this is generally a good practice to avoid unnecessary re-renders, consider if the complexity added byuseCallbackis justified given the simplicity of the function.
96-158: Review the main render block for potential optimizations.The main render block of the
EditorTabscomponent is quite dense and handles multiple conditions. Consider breaking it down into smaller, more manageable components or using more descriptive helper functions to improve readability.app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2)
41-41: Ensure proper import usage.The import of
QueriesBlankStateis added. Ensure that this component is used appropriately within the file and that its import is necessary.
Line range hint
215-215: Avoid redundant double-negation.The use of double-negation (
!!) is unnecessary and can be simplified.- description: !!fileOperation.isBeta ? ( + description: fileOperation.isBeta ? (
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/EditorPaneSegments.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx (3 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.tsx (4 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/MainPane/index.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/hooks.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/IDE/hooks.ts
Additional context used
Biome
app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
[error] 215-215: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
Additional comments not posted (9)
app/client/src/pages/Editor/IDE/MainPane/index.tsx (3)
6-10: Ensure that the new imports are used appropriately within the file.
15-15: The usage ofuseSelectorto fetchideViewModeis correct and follows Redux best practices.
25-25: Conditional rendering based onideViewModeis implemented correctly. This ensures thatEditorTabsis only rendered inFullScreenmode.app/client/src/pages/Editor/IDE/EditorPane/EditorPaneSegments.tsx (3)
Line range hint
9-22: The imports and constants are correctly added and used. Ensure thatEditorViewModeandgetIDEViewModeare properly utilized in the component logic.
26-26: TheideViewModeis fetched correctly using Redux'suseSelector. This is a good practice for managing state in React applications.
37-37: The conditional rendering logic is correct, ensuring thatEditorTabsis only rendered inSplitScreenmode.app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx (1)
21-21: The addition of thecurrentTabprop in the test cases is appropriate. It ensures that the tests reflect the updated component interface and functionality.Also applies to: 44-44, 59-59
app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.tsx (2)
21-22: The addition ofcurrentTabandnewTabClickCallbackprops is correctly handled in the component's destructuring and usage. This enhances the component's flexibility and functionality.Also applies to: 28-29
70-70: The conditional class application based oncurrentTaband the handling of the 'New Tab' click are implemented correctly. These changes improve user interaction and component responsiveness.Also applies to: 89-91
| } else if (showListView) { | ||
| setShowListView(false); | ||
| } | ||
| }, [files, segmentMode]); | ||
|
|
||
| const toggleEditorMode = useCallback(() => { | ||
| const newMode = | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? EditorViewMode.FullScreen | ||
| : EditorViewMode.SplitScreen; | ||
|
|
||
| AnalyticsUtil.logEvent("EDITOR_MODE_CHANGE", { | ||
| to: newMode, | ||
| }); | ||
| dispatch(setIdeEditorViewMode(newMode)); | ||
| }, [ideViewMode, dispatch]); | ||
|
|
||
| const handleHamburgerClick = () => { | ||
| if (files.length === 0 && segmentMode !== EditorEntityTabState.Add) return; | ||
| setShowListView(!showListView); | ||
| }; | ||
|
|
||
| if (!isSideBySideEnabled) return null; | ||
| if (segment === EditorEntityTab.UI) return null; | ||
|
|
||
| const onTabClick = (tab: EntityItem) => { | ||
| setShowListView(false); | ||
| tabClickHandler(tab); | ||
| }; | ||
|
|
||
| const newTabClickHandler = () => { | ||
| setShowListView(false); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <Container> | ||
| {ideViewMode === EditorViewMode.SplitScreen && ( | ||
| <ToggleButton | ||
| icon="hamburger" | ||
| isSelected={showListView} | ||
| onClick={handleHamburgerClick} | ||
| size="md" | ||
| /> | ||
| )} | ||
| <FileTabs | ||
| currentTab={activeTab} | ||
| navigateToTab={onTabClick} | ||
| newTabClickCallback={newTabClickHandler} | ||
| onClose={closeClickHandler} | ||
| tabs={files} | ||
| /> | ||
| {files.length > 0 ? <AddButton /> : null} | ||
| <Tooltip | ||
| content={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? createMessage(MAXIMIZE_BUTTON_TOOLTIP) | ||
| : createMessage(MINIMIZE_BUTTON_TOOLTIP) | ||
| } | ||
| > | ||
| <Button | ||
| className="ml-auto !min-w-[24px]" | ||
| data-testid={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "t--ide-maximize" | ||
| : "t--ide-minimize" | ||
| } | ||
| id={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "editor-mode-maximize" | ||
| : "editor-mode-minimize" | ||
| } | ||
| isIconButton | ||
| kind="tertiary" | ||
| onClick={toggleEditorMode} | ||
| startIcon={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "maximize-v3" | ||
| : "minimize-v3" | ||
| } | ||
| /> | ||
| </Tooltip> | ||
| </Container> | ||
| {showListView && ideViewMode === EditorViewMode.SplitScreen && ( | ||
| <ListContainer | ||
| bg="var(--ads-v2-color-bg)" | ||
| className="absolute top-[78px] albin" | ||
| h="calc(100% - 78px)" | ||
| w="100%" | ||
| zIndex="10" | ||
| > | ||
| <ListQuery /> | ||
| </ListContainer> | ||
| )} | ||
| {ideViewMode === EditorViewMode.SplitScreen && <Announcement />} | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default EditorTabs; |
There was a problem hiding this comment.
Refactor suggestion for EditorTabs component.
The EditorTabs component is quite large and handles multiple responsibilities, including state management, event handling, and UI rendering. Consider breaking it down into smaller, more manageable components or hooks to improve readability and maintainability.
There was a problem hiding this comment.
I agree here. We should break up things like overflow, add and the EditorMode Toggle buttons
There was a problem hiding this comment.
@hetunandu, great to hear that you agree with the refactoring suggestion! Here’s a possible approach to breaking down the EditorTabs component:
- Overflow Handling: Create a
TabOverflowHandlercomponent that manages the visibility and interactions of tabs when they exceed the available space. - Add Button: This can be encapsulated into an
AddTabButtoncomponent that handles the logic and UI for adding new tabs. - Editor Mode Toggle: A
ToggleEditorModeButtoncomponent could be responsible for switching between different editor modes and encapsulating the associated logic and UI.
Each component can then be used within EditorTabs to clean up the main component and make each piece easier to manage and test independently. If you need further detailed breakdowns or assistance with this, feel free to ask!
| if (!isSideBySideEnabled) return null; | ||
| if (segment === EditorEntityTab.UI) return null; |
There was a problem hiding this comment.
Handle conditional rendering more explicitly.
The return of null based on isSideBySideEnabled could be handled more explicitly at the beginning of the component to avoid deep nesting and improve readability.
+ if (!isSideBySideEnabled || segment === EditorEntityTab.UI) return null;
- if (!isSideBySideEnabled) return null;
- if (segment === EditorEntityTab.UI) return null;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.
| if (!isSideBySideEnabled) return null; | |
| if (segment === EditorEntityTab.UI) return null; | |
| if (!isSideBySideEnabled || segment === EditorEntityTab.UI) return null; |
|
Deploy-Preview-URL: https://ce-34150.dp.appsmith.com |
|
While testing I found that the overflow list on JS segment also shows API overflow list. |
| } else if (showListView) { | ||
| setShowListView(false); | ||
| } | ||
| }, [files, segmentMode]); | ||
|
|
||
| const toggleEditorMode = useCallback(() => { | ||
| const newMode = | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? EditorViewMode.FullScreen | ||
| : EditorViewMode.SplitScreen; | ||
|
|
||
| AnalyticsUtil.logEvent("EDITOR_MODE_CHANGE", { | ||
| to: newMode, | ||
| }); | ||
| dispatch(setIdeEditorViewMode(newMode)); | ||
| }, [ideViewMode, dispatch]); | ||
|
|
||
| const handleHamburgerClick = () => { | ||
| if (files.length === 0 && segmentMode !== EditorEntityTabState.Add) return; | ||
| setShowListView(!showListView); | ||
| }; | ||
|
|
||
| if (!isSideBySideEnabled) return null; | ||
| if (segment === EditorEntityTab.UI) return null; | ||
|
|
||
| const onTabClick = (tab: EntityItem) => { | ||
| setShowListView(false); | ||
| tabClickHandler(tab); | ||
| }; | ||
|
|
||
| const newTabClickHandler = () => { | ||
| setShowListView(false); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <Container> | ||
| {ideViewMode === EditorViewMode.SplitScreen && ( | ||
| <ToggleButton | ||
| icon="hamburger" | ||
| isSelected={showListView} | ||
| onClick={handleHamburgerClick} | ||
| size="md" | ||
| /> | ||
| )} | ||
| <FileTabs | ||
| currentTab={activeTab} | ||
| navigateToTab={onTabClick} | ||
| newTabClickCallback={newTabClickHandler} | ||
| onClose={closeClickHandler} | ||
| tabs={files} | ||
| /> | ||
| {files.length > 0 ? <AddButton /> : null} | ||
| <Tooltip | ||
| content={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? createMessage(MAXIMIZE_BUTTON_TOOLTIP) | ||
| : createMessage(MINIMIZE_BUTTON_TOOLTIP) | ||
| } | ||
| > | ||
| <Button | ||
| className="ml-auto !min-w-[24px]" | ||
| data-testid={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "t--ide-maximize" | ||
| : "t--ide-minimize" | ||
| } | ||
| id={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "editor-mode-maximize" | ||
| : "editor-mode-minimize" | ||
| } | ||
| isIconButton | ||
| kind="tertiary" | ||
| onClick={toggleEditorMode} | ||
| startIcon={ | ||
| ideViewMode === EditorViewMode.SplitScreen | ||
| ? "maximize-v3" | ||
| : "minimize-v3" | ||
| } | ||
| /> | ||
| </Tooltip> | ||
| </Container> | ||
| {showListView && ideViewMode === EditorViewMode.SplitScreen && ( | ||
| <ListContainer | ||
| bg="var(--ads-v2-color-bg)" | ||
| className="absolute top-[78px] albin" | ||
| h="calc(100% - 78px)" | ||
| w="100%" | ||
| zIndex="10" | ||
| > | ||
| <ListQuery /> | ||
| </ListContainer> | ||
| )} | ||
| {ideViewMode === EditorViewMode.SplitScreen && <Announcement />} | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| export default EditorTabs; |
There was a problem hiding this comment.
I agree here. We should break up things like overflow, add and the EditorMode Toggle buttons
…o overflow-tabs-list-view
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/List.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx
Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx
Additional comments not posted (3)
app/client/src/pages/Editor/IDE/EditorTabs/List.tsx (2)
10-19: The styling inListContaineris well-defined and adheres to design specifications.
21-35: The conditional rendering logic in theListcomponent is clear and functional. Ensure that the segment definitions (EditorEntityTab.QUERIESand others) are consistently used across the application.Verification successful
The provided script encountered regex errors due to the inclusion of characters like
],and}, which are not part of the segment names. This led to incorrect parsing of the segment definitions. To address this, we need to refine the script to correctly extract and verify the segment definitions.Next Steps
- Correct the Regex to Extract Segment Names Properly: Adjust the regex pattern to accurately capture the segment names without trailing characters.
- Re-run the Verification: Use the refined script to verify the consistent use of segment definitions across the codebase.
Refined Shell Script
Let's refine and re-run the shell script to ensure accurate extraction and verification of segment definitions.
The refined script successfully extracted and verified the segment definitions and their usage across the codebase. The segments
EditorEntityTab.JS,EditorEntityTab.QUERIES, andEditorEntityTab.UIwere consistently used, confirming that the conditional rendering logic in theListcomponent is correctly implemented.
- The segments are defined and used in various files such as
selectors,reducers,hooks, and test files.- No inconsistencies were found in the usage of these segments.
This confirms that the segment definitions are consistently used across the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistent use of segment definitions across the application. # Search for `EditorEntityTab.QUERIES` and other segment definitions to ensure consistent usage. rg --type typescript "EditorEntityTab.QUERIES"Length of output: 81
Script:
#!/bin/bash # Description: Verify the consistent use of segment definitions across the application. # Step 1: Search for all segment definitions in EditorEntityTab. rg --type ts "EditorEntityTab" > segment_definitions.txt # Step 2: Extract segment definitions and search for their usages. while read -r line; do if [[ $line =~ EditorEntityTab\.(.*) ]]; then segment=${BASH_REMATCH[1]} rg --type ts "EditorEntityTab.$segment" >> segment_usages.txt fi done < segment_definitions.txt # Output the results for review. cat segment_definitions.txt cat segment_usages.txtLength of output: 14873
Script:
#!/bin/bash # Description: Verify the consistent use of segment definitions across the application. # Step 1: Search for all segment definitions in EditorEntityTab. rg --type ts "EditorEntityTab" > segment_definitions.txt # Step 2: Extract segment definitions and search for their usages. while read -r line; do if [[ $line =~ EditorEntityTab\.([A-Z_]+) ]]; then segment=${BASH_REMATCH[1]} rg --type ts "EditorEntityTab.$segment" >> segment_usages.txt fi done < segment_definitions.txt # Output the results for review. cat segment_definitions.txt cat segment_usages.txtLength of output: 17043
app/client/src/pages/Editor/IDE/EditorTabs/ScreenModeToggle.tsx (1)
15-62: TheScreenModeTogglecomponent is implemented correctly, using best practices for state management and UI responsiveness. Verify that thecreateMessagefunction properly handles dynamic content for tooltips.Verification successful
The search results indicate that the
createMessagefunction is used extensively throughout the codebase. Given this wide usage, it is crucial to ensure thatcreateMessageis correctly implemented to handle dynamic content appropriately.To verify this, we need to locate the implementation of the
createMessagefunction and review its code. This will help us determine if it can handle dynamic content as expected.
The previous script did not locate the implementation of the
createMessagefunction. To proceed, we should refine our search approach. Instead of looking for a function definition, we can search for the export statement related tocreateMessageto identify its module and then inspect the module for the function implementation.
The
createMessagefunction is exported from three different files:
app/client/src/ce/constants/messages.tsapp/client/packages/design-system/widgets-old/src/constants/messages.tsxapp/client/cypress/support/Objects/CommonErrorMessages.tsTo verify the correct handling of dynamic content, we need to inspect the implementation of
createMessagein these files.
The
createMessagefunction implementation in all three files is as follows:export function createMessage( format: (...strArgs: any[]) => string, ...args: any[] ) { return format(...args); }This function takes a formatting function (
format) and a list of arguments (args). It applies the formatting function to the arguments, which suggests that it can handle dynamic content as long as the provided formatting function is capable of doing so.Given this, the
createMessagefunction appears to be correctly implemented to handle dynamic content. Therefore, theScreenModeTogglecomponent's use ofcreateMessagefor tooltips is appropriate and should work as expected.
- : The
createMessagefunction is implemented correctly to handle dynamic content, ensuring that theScreenModeTogglecomponent's tooltips will display the appropriate messages.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of dynamic content in `createMessage`. # Search for the `createMessage` function usage and ensure it handles dynamic content appropriately. rg --type typescript "createMessage"Length of output: 71
Script:
#!/bin/bash # Description: Verify the correct handling of dynamic content in `createMessage`. # Search for the `createMessage` function usage and ensure it handles dynamic content appropriately. rg --type ts "createMessage"Length of output: 205584
Script:
#!/bin/bash # Description: Locate the implementation of the `createMessage` function to verify its handling of dynamic content. # Search for the definition of the `createMessage` function in the TypeScript files. ast-grep --lang ts --pattern 'function createMessage($$) { $$ }'Length of output: 66
Script:
#!/bin/bash # Description: Locate the module where `createMessage` is exported to verify its implementation. # Search for the export statement of `createMessage` in the TypeScript files. rg --type ts "export .* createMessage"Length of output: 304
Script:
#!/bin/bash # Description: Inspect the implementation of `createMessage` in the identified files to verify its handling of dynamic content. # Display the implementation of `createMessage` from each identified file. for file in app/client/src/ce/constants/messages.ts app/client/packages/design-system/widgets-old/src/constants/messages.tsx app/client/cypress/support/Objects/CommonErrorMessages.ts; do echo "File: $file" rg --pcre2 --context 10 "function createMessage" $file doneLength of output: 2257
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- app/client/src/IDE/Components/FileTab.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/EditorPaneSegments.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/AddButton.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/Container.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/ListButton.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/SearchableFilesList.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/StyledComponents.tsx (2 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- app/client/src/pages/Editor/IDE/EditorTabs/Container.tsx
- app/client/src/pages/Editor/IDE/EditorTabs/StyledComponents.tsx
Files skipped from review as they are similar to previous changes (3)
- app/client/src/pages/Editor/IDE/EditorPane/EditorPaneSegments.tsx
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.tsx
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx
Additional comments not posted (8)
app/client/src/pages/Editor/IDE/EditorTabs/ListButton.tsx (2)
12-12: Updated import forListTitlealigns with the new design requirements for the overflow tabs list view.
53-53: Usage ofListTitlein the JSX ensures the overflow tabs' titles are displayed as per the new UI specifications.app/client/src/pages/Editor/IDE/EditorTabs/AddButton.tsx (3)
1-2: ImportingforwardRefis essential for the new functionality allowingAddButtonto accept refs, enhancing its reusability and integration with other components.
12-65: Proper use offorwardRefto enhance component integration with other parts of the application by allowing it to handle refs.
15-19: Addition ofnewTabClickCallbackandonCloseprops enhances the component's functionality by handling tab creation and closing actions dynamically.app/client/src/IDE/Components/FileTab.tsx (1)
1-97: TheFileTabcomponent is well-implemented, with appropriate use of styled components, conditional rendering, and event handling, aligning with the design specifications.app/client/src/pages/Editor/IDE/EditorTabs/SearchableFilesList.tsx (2)
16-16: Updated import forListTitlesupports the new design requirements for the searchable files list.
67-67: Correct usage ofListTitlein the JSX ensures that file titles are displayed in line with the new UI specifications.
…o overflow-tabs-list-view
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/pages/Editor/IDE/EditorTabs/AddButton.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/IDE/EditorTabs/AddButton.tsx
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IDE/EditorTabs/index.tsx
Additional comments not posted (5)
app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (5)
1-1: Imports are correctly specified and organized.Also applies to: 3-4
10-16: Component definition and props are correctly implemented with appropriate types.
19-19: Conditional rendering based onsegmentModeis implemented correctly.
21-24: Event handling logic inonCloseClickis correctly implemented to stop event propagation and handle the close action.
36-36: Export statement for theAddTabcomponent is correctly implemented.
albinAppsmith
added
ok-to-test
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- app/client/cypress/e2e/Regression/ClientSide/IDE/IDE_Add_Pane_Interactions_spec.ts (1 hunks)
- app/client/cypress/support/Pages/IDE/FileTabs.ts (1 hunks)
- app/client/src/IDE/Components/FileTab.tsx (1 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (4 hunks)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (17 hunks)
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx (5 hunks)
- app/client/src/utils/URLUtils.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
Files skipped from review as they are similar to previous changes (2)
- app/client/src/IDE/Components/FileTab.tsx
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx
Additional context used
Biome
app/client/src/utils/URLUtils.ts
[error] 21-23: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (5)
app/client/cypress/support/Pages/IDE/FileTabs.ts (2)
2-2: LGTM: Improved consistency in tab identifiers.The use of
sanitizeStringfromURLUtilsto standardize tab names in Cypress locators is a good practice. This change enhances the consistency and reliability of tests by ensuring that tab identifiers are uniformly formatted.Also applies to: 6-7
Line range hint
2-30: Good use of Cypress best practices inFileTabsclass.The methods within the
FileTabsclass are well-implemented, making good use of theObjectsRegistryfor DOM interactions and handling various tab-related operations effectively.app/client/src/utils/URLUtils.ts (1)
43-45: Well-implemented string sanitization utility.The
sanitizeStringfunction is effectively designed to ensure string values are safe and consistent for use as identifiers or parts of URLs by converting to lowercase and replacing non-alphanumeric characters with underscores.app/client/cypress/e2e/Regression/ClientSide/IDE/IDE_Add_Pane_Interactions_spec.ts (1)
71-71: Consistent usage of tab naming in tests.The update to use
"new_query"in thecloseTabmethod call aligns with the standardized naming conventions, ensuring consistency across the test suite.app/client/src/pages/Editor/IDE/EditorPane/JS/JSRender.test.tsx (1)
154-154: Enhanced assertions for active tab state in tests.The updates to assert the active state of JavaScript object tabs in various scenarios enhance the robustness of the tests. This is crucial for ensuring that the UI correctly reflects the expected state based on user interactions or state changes.
Also applies to: 204-204, 248-248, 286-286
…o overflow-tabs-list-view
albinAppsmith
added
ok-to-test
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx (4 hunks)
Additional comments not posted (4)
app/client/src/pages/Editor/IDE/EditorTabs/FileTabs.test.tsx (4)
4-7: Review of new imports:The imports at lines 4 and 7 have been added to support the new functionalities and data structures used in the tests. These imports are correctly placed and are necessary for the test's functionality.
19-24: Review ofactiveEntitysetup:The
activeEntityobject setup is well-defined and matches the expected structure required by theFileTabscomponent. This setup is crucial for testing the component with different states.
29-38: Review of the test case "renders tabs correctly":This test verifies that each tab is rendered with the correct content. The use of
sanitizeStringfor generating test IDs is a good practice as it ensures that the IDs are consistent and valid HTML attributes. The test is well-structured and covers the basic rendering logic of the component.
66-74: Review of the test case "check for close click":This test effectively simulates a user clicking the close button on a tab and checks if the
onClosefunction is called with the correct key. The test is well-structured and provides necessary coverage for the close functionality of the tabs.
[APROVED]
This reverts commit 519b53e.
## Description This PR implements the new design for the list view. Fixes appsmithorg#33432 ## Automation /ok-to-test tags="@tag.Sanity, @tag.IDE" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9660135881> > Commit: fb8addb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9660135881&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.IDE` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced `AddTab` component to add new JavaScript or Query tabs. - Added `ScreenModeToggle` for switching between full-screen and split-screen modes. - Added `FileTab` component for improved tab interactions. - Introduced `List` component for conditional rendering based on editor state. - **Bug Fixes** - Corrected test assertions and tab names in `JSRender.test.tsx` and `QueryRender.test.tsx`. - Fixed tab closure and interaction flow in `IDE_Add_Pane_Interactions_spec.ts`. - **Refactor** - Simplified selector functions and updated component imports for better readability and performance. - **Tests** - Updated tests to include `currentEntity` props and use `sanitizeString` for tab titles. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
This PR implements the new design for the list view.
Fixes #33432
Automation
/ok-to-test tags="@tag.Sanity, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9660135881
Commit: fb8addb
Cypress dashboard.
Tags:
@tag.Sanity, @tag.IDECommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
AddTabcomponent to add new JavaScript or Query tabs.ScreenModeTogglefor switching between full-screen and split-screen modes.FileTabcomponent for improved tab interactions.Listcomponent for conditional rendering based on editor state.Bug Fixes
JSRender.test.tsxandQueryRender.test.tsx.IDE_Add_Pane_Interactions_spec.ts.Refactor
Tests
currentEntityprops and usesanitizeStringfor tab titles.