chore: entity tabs replacement #38989

alex-golovanov · 2025-02-03T20:30:00Z

Description

Replacing entity tab bar and components with ADS templates.

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13172995565
Commit: f3db2d9
Cypress dashboard.
Tags: @tag.All
Spec:

Thu, 06 Feb 2025 07:56:59 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

Yes
No

Summary by CodeRabbit

New Features
- Enhanced tab interaction: The add button now supports dynamic visibility and a loading indicator when new tabs are being created.
- Improved editing: Tab labels now feature refined controls for entering and exiting edit mode, offering a more flexible user experience.
- New EntityTabsHeader component added to enhance tab management.
- Updated tab structure to utilize DismissibleTab for improved functionality.
- Introduced new props for DismissibleTabBar to allow for additional styling and control over add button visibility.
- New properties added to DismissibleTab for greater interactivity and customization.
- Additional properties introduced to EditableDismissibleTab for enhanced editing capabilities.
Refactor
- Streamlined the tab interface by replacing legacy components with modern, responsive alternatives, simplifying the overall design and interaction.
- Enhanced event handling and state management for better performance and user experience.
- Updated import paths and component structures for clarity and maintainability.

…able-tabs-list

# Conflicts: # app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx # app/client/packages/design-system/ads/src/DismissibleTab/index.ts

…ssibleTabBar.styles.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

…-tabs-header

# Conflicts: # app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx # app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts # app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx

…tabs-replacement

# Conflicts: # app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.mdx

- Delete AddButton, Container, FileTab, and their associated tests and styles. - Replace them with DismissibleTab and EntityTabsHeader components for improved tab management. - Adjust logic to manage tab visibility and interactions, simplifying the code structure. - Ensure seamless integration of newly added components into the EditorTabs functionality.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (1)
104-119: Good loading state implementation.

The conditional rendering between Spinner and PlusButton provides clear user feedback. However, consider adding aria-label to the Spinner for better accessibility.
-            <Spinner size="md" />
+            <Spinner aria-label="Adding new tab" size="md" />

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c92bef9 and 4c80891.

📒 Files selected for processing (5)

app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (3 hunks)
app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (2 hunks)
app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx (3 hunks)
app/client/src/pages/Editor/IDE/EditorTabs/Editortabs.test.tsx (2 hunks)
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

app/client/src/pages/Editor/IDE/EditorTabs/EditableTab.tsx

⏰ Context from checks skipped due to timeout of 90000ms (9)

GitHub Check: perform-test / rts-build / build
GitHub Check: perform-test / server-build / server-unit-tests
GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
GitHub Check: client-lint / client-lint
GitHub Check: client-build / client-build
GitHub Check: client-prettier / prettier-check
GitHub Check: chromatic-deployment
GitHub Check: chromatic-deployment
GitHub Check: storybook-tests

🔇 Additional comments (7)

app/client/src/pages/Editor/IDE/EditorTabs/AddTab.tsx (2)

2-4: Good use of useEventCallback for event handler optimization!

The event handler is now memoized properly, which helps prevent unnecessary re-renders.

Also applies to: 24-27

34-41: Clean migration to DismissibleTab component.

The component migration maintains all necessary functionality while aligning with the new design system.

app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (1)

24-25: Well-structured prop additions.

The new props enhance component flexibility while maintaining clean defaults.

app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (3)

54-68: Well-organized effect hooks with clear naming.

The effects are now properly named and focused, improving code readability and maintainability.

76-83: TODO comment needs attention.

The comment indicates that handleTabClick returns a new function every time. Consider implementing the suggested recomposition.

Would you like me to help implement a memoized version of this handler?

102-132: Clean implementation of DismissibleTabBar with proper prop handling.

The tab rendering logic is well-structured and maintains all necessary functionality.

app/client/src/pages/Editor/IDE/EditorTabs/Editortabs.test.tsx (1)

47-48: Improved test assertions with clear documentation.

The updated assertions and comments clearly explain the expected behavior with sentinel divs.

Also applies to: 73-74

github-actions · 2025-02-04T11:02:29Z

🔴🔴🔴 Cyclic Dependency Check:

This PR has increased the number of cyclic dependencies by 4, when compared with the release branch.

Refer this document to identify the cyclic dependencies introduced by this PR.

coderabbitai

Actionable comments posted: 0

🔭 Outside diff range comments (1)

app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (1)

76-83: Optimize tab click handler to prevent unnecessary recreations.

The TODO comment indicates a performance concern. Consider using useCallback with a dependency array or moving the handler to a custom hook.

+const useTabClickHandler = (
+  dispatch: Dispatch,
+  tabClickHandler: (tab: EntityItem) => void,
+) =>
+  useCallback(
+    (tab: EntityItem) => {
+      dispatch(setListViewActiveState(false));
+      tabClickHandler(tab);
+    },
+    [dispatch, tabClickHandler],
+  );

 // In component:
-const handleTabClick = useCallback(
-  (tab: EntityItem) => () => {
-    dispatch(setListViewActiveState(false));
-    tabClickHandler(tab);
-  },
-  [dispatch, tabClickHandler],
-);
+const handleTabClick = useTabClickHandler(dispatch, tabClickHandler);

🧹 Nitpick comments (11)

app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts (2)
22-23: Consider using a single height property.

Since both min-height and max-height are set to 32px, this effectively creates a fixed height. Consider using just height: 32px for simplicity.
-  max-height: 32px;
-  min-height: 32px;
+  height: 32px;
45-45: Consider consistency with height properties.

For consistency with the root component's height constraints, consider whether this should also use both min and max height properties.
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.types.ts (1)
4-14: Add JSDoc comments for better documentation.

The interface is well-structured with proper types. Consider adding JSDoc comments to document the purpose of each prop, especially the new ones.
 export interface DismissibleTabBarProps {
+  /** Children can be DismissibleTab elements or any valid React node */
   children:
     | React.ReactElement<DismissibleTabProps>
     | React.ReactElement<DismissibleTabProps>[]
     | React.ReactNode;
+  /** Additional CSS class name */
   className?: string;
+  /** Whether to disable the add button */
   disableAdd?: boolean;
+  /** Whether to hide the add button */
   hideAdd?: boolean;
+  /** Whether a new tab is currently being added */
   isAddingNewTab?: boolean;
+  /** Callback when add button is clicked */
   onTabAdd: () => void;
 }
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.styles.ts (1)
24-26: LGTM! Consider documenting the negative margin.

The negative margin is used to handle border overlap. Consider adding a comment to explain this visual adjustment for future maintainers.
 export const TabBar = styled(DismissibleTabBar)`
+  /* Negative margin compensates for the border overlap with the container */
   margin-bottom: -1px;
 `;
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.tsx (1)
32-34: Add component documentation.

The implementation is clean. Consider adding JSDoc documentation to describe the component's purpose and usage.
+/**
+ * EntityTabBar is a wrapper around DismissibleTabBar with custom styling
+ * from the Appsmith Design System.
+ */
 export const EntityTabBar = (props: DismissibleTabBarProps) => {
   return <Styled.TabBar {...props} />;
 };
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx (2)
28-36: Consider using a custom hook for state management.

The state management logic could be extracted into a custom hook to improve reusability and reduce complexity in the component.
+const useEditableState = (
+  propIsEditing?: boolean,
+  propOnEnterEditMode?: () => void,
+  propOnExitEditMode?: () => void,
+) => {
+  const {
+    setFalse: localOnExitEditMode,
+    setTrue: localOnEnterEditMode,
+    value: localIsEditing,
+  } = useBoolean(false);
+
+  return {
+    isEditing: propIsEditing ?? localIsEditing,
+    handleEnterEditMode: propOnEnterEditMode ?? localOnEnterEditMode,
+    handleExitEditMode: propOnExitEditMode ?? localOnExitEditMode,
+  };
+};
28-36: Consider using a custom hook for state management.

The state management logic could be extracted into a custom hook to improve reusability and reduce complexity.
+// In a new file: useEditableState.ts
+export const useEditableState = (
+  propIsEditing?: boolean,
+  propOnEnterEditMode?: () => void,
+  propOnExitEditMode?: () => void,
+) => {
+  const {
+    setFalse: localOnExitEditMode,
+    setTrue: localOnEnterEditMode,
+    value: localIsEditing,
+  } = useBoolean(false);
+
+  return {
+    isEditing: propIsEditing ?? localIsEditing,
+    handleEnterEditMode: propOnEnterEditMode ?? localOnEnterEditMode,
+    handleExitEditMode: propOnExitEditMode ?? localOnExitEditMode,
+  };
+};

// In EditableDismissibleTab.tsx
-  const {
-    setFalse: localOnExitEditMode,
-    setTrue: localOnEnterEditMode,
-    value: localIsEditing,
-  } = useBoolean(false);
-
-  const isEditing = propIsEditing ?? localIsEditing;
-  const handleEnterEditMode = propOnEnterEditMode ?? localOnEnterEditMode;
-  const handleExitEditMode = propOnExitEditMode ?? localOnExitEditMode;
+  const { handleEnterEditMode, handleExitEditMode, isEditing } = useEditableState(
+    propIsEditing,
+    propOnEnterEditMode,
+    propOnExitEditMode,
+  );
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (2)
18-20: Consider using CSS-in-JS for consistent styling.

The hardcoded height value could be moved to the styled components to maintain consistency with the rest of the codebase.
-const SCROLL_AREA_STYLE = {
-  height: 32,
-};
+const ScrollAreaWrapper = styled(ScrollArea)`
+  height: 32px;
+`;
18-20: Extract magic number into a named constant.

The height value should be defined as a named constant for better maintainability.
+const TAB_BAR_HEIGHT = 32;
+
 const SCROLL_AREA_STYLE = {
-  height: 32,
+  height: TAB_BAR_HEIGHT,
 };
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2)
93-134: Consider extracting tab rendering logic.

The tab rendering logic could be extracted into a separate component to improve readability and maintainability.
+const TabList = ({ files, entities, currentEntity, segmentMode, handleTabClick, closeClickHandler }) => (
+  <>
+    {files.map((tab) => {
+      const entity = entities.find((entity) => entity.key === tab.key);
+      return (
+        <EditableTab
+          entity={entity}
+          icon={tab.icon}
+          id={tab.key}
+          isActive={
+            currentEntity.id === tab.key &&
+            segmentMode !== EditorEntityTabState.Add &&
+            !isListViewActive
+          }
+          key={tab.key}
+          onClick={handleTabClick(tab)}
+          onClose={closeClickHandler}
+          title={tab.title}
+        />
+      );
+    })}
+  </>
+);
115-119: Extract complex boolean condition into a descriptive variable.

The isActive prop contains multiple conditions that could be more readable if extracted.
+const isTabActive = (
+  currentEntityId: string,
+  tabKey: string,
+  segmentMode: EditorEntityTabState,
+  isListViewActive: boolean,
+) =>
+  currentEntityId === tabKey &&
+  segmentMode !== EditorEntityTabState.Add &&
+  !isListViewActive;

// In JSX:
-isActive={
-  currentEntity.id === tab.key &&
-  segmentMode !== EditorEntityTabState.Add &&
-  !isListViewActive
-}
+isActive={isTabActive(
+  currentEntity.id,
+  tab.key,
+  segmentMode,
+  isListViewActive,
+)}

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c80891 and b01b66c.

📒 Files selected for processing (24)

app/client/cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js (1 hunks)
app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts (3 hunks)
app/client/cypress/e2e/Regression/ClientSide/JSObject/RenameJSObjectBug38207_spec.ts (2 hunks)
app/client/cypress/e2e/Regression/ServerSide/Datasources/Oracle_Spec.ts (2 hunks)
app/client/cypress/support/ApiCommands.js (1 hunks)
app/client/cypress/support/Objects/CommonLocators.ts (1 hunks)
app/client/cypress/support/Pages/AggregateHelper.ts (2 hunks)
app/client/cypress/support/Pages/ApiPage.ts (1 hunks)
app/client/cypress/support/Pages/JSEditor.ts (2 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.stories.tsx (1 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.styles.ts (2 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx (1 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx (1 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts (3 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (4 hunks)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.types.ts (1 hunks)
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx (1 hunks)
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx (3 hunks)
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.stories.tsx (3 hunks)
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.styles.ts (2 hunks)
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.tsx (2 hunks)
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.types.ts (2 hunks)
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/index.ts (1 hunks)
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (3 hunks)

✅ Files skipped from review due to trivial changes (4)

app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.stories.tsx
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx

🚧 Files skipped from review as they are similar to previous changes (2)

app/client/cypress/support/Pages/JSEditor.ts
app/client/packages/design-system/ads/src/Templates/EntityTabsHeader/EntityTabsHeader.types.ts

🧰 Additional context used

📓 Path-based instructions (8)

app/client/cypress/support/ApiCommands.js (1)