Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Tab navigation issue in multi page apps #38272

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

hetunandu
Copy link
Member

@hetunandu hetunandu commented Dec 20, 2024

Description

Fixes the caching in useEventCallback for tabClickHandler that was getting the old pageId

Fixes #38216

Automation

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

🔍 Cypress test results

Tip

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


Fri, 20 Dec 2024 10:53:24 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Introduced Cypress end-to-end tests for tab navigation within the IDE, ensuring seamless switching between JavaScript and query tabs.
    • Added a new function editorTabSelector for generating string selectors for IDE tabs.
  • Bug Fixes

    • Optimized tab click handling in the editor for improved performance.
  • Chores

    • Updated imports to include necessary hooks for better functionality.

Copy link
Contributor

coderabbitai bot commented Dec 20, 2024

Walkthrough

The pull request introduces optimizations to the EditorTabs component in index.tsx by replacing the useEventCallback hook with useCallback for the handleTabClick function. This change aims to enhance performance through memoization and explicitly defines its dependencies as [dispatch, tabClickHandler]. Additionally, a new file Tabs_Navigation_spec.ts has been added, containing Cypress end-to-end tests for verifying tab navigation functionalities in the IDE. The overall functionality of the component remains unchanged.

Changes

File Change Summary
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx - Replaced useEventCallback with useCallback for handleTabClick
- Defined dependencies for memoization
app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts - Introduced Cypress tests for tab navigation functionalities in the IDE
app/client/cypress/support/Pages/EditorNavigation.ts - Added new method editorTabSelector(name: string) for generating tab selectors

Assessment against linked issues

Objective Addressed Explanation
Navigation between JSObjects [#38216] The memoization change may impact navigation, but further testing is needed to confirm resolution of the issue.

Possibly related PRs

Suggested labels

High, User Testing

Suggested reviewers

  • ankitakinger
  • albinAppsmith
  • ApekshaBhosale

Poem

🖥️ In the realm of tabs and code so bright,
A callback dance of memoized might,
Optimizing clicks with gentle care,
Performance singing beyond compare,
Where functions flow and logic gleams! 🚀


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?

❤️ Share
🪧 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 @coderabbitai in 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 @coderabbitai in 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 pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere 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.

@github-actions github-actions bot added Bug Something isn't working Community Reported issues reported by community members IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product Medium Issues that frustrate users due to poor UX Needs Triaging Needs attention from maintainers to triage Production QA Needs QA attention QA Pod Issues under the QA Pod labels Dec 20, 2024
@hetunandu hetunandu added the ok-to-test Required label for CI label Dec 20, 2024
@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12426966500.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 38272.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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

89-95: Good fix, with room for optimization

The switch to useCallback with proper dependencies fixes the caching issue. However, consider optimizing further:

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

Then update the onClick prop:

onClick={() => handleTabClick(tab)}

This avoids creating a new closure for each tab while maintaining the same functionality.

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 71c5171 and ac94099.

📒 Files selected for processing (1)
  • app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2 hunks)
🔇 Additional comments (2)
app/client/src/pages/Editor/IDE/EditorTabs/index.tsx (2)

1-1: LGTM: Clean import addition

The useCallback import is properly grouped with other React hooks.


89-95: Verify similar patterns in the codebase

Let's check for similar patterns that might need the same fix.

✅ Verification successful

No issues found with the callback pattern

The code follows a consistent pattern across the codebase. The handleTabClick implementation correctly:

  1. Uses useCallback with proper dependencies
  2. Dispatches the list view state update
  3. Calls the tabClickHandler from the IDE hooks which handles navigation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other useEventCallback instances that might need similar fixes
rg "useEventCallback.*tabClickHandler" -A 3

# Search for other components using tabClickHandler
ast-grep --pattern 'const { $$$, tabClickHandler, $$$ } = use$_'

Length of output: 112


Script:

#!/bin/bash
# Search for tabClickHandler usage in the codebase
rg "tabClickHandler" -A 3

# Search for similar callback patterns with dispatch and state updates
ast-grep --pattern 'const $_ = useCallback(($$$) => () => {
  dispatch($_);
  $$$
}, $_)'

# Find where tabClickHandler is defined or passed as prop
ast-grep --pattern 'tabClickHandler: $_'

Length of output: 113019


Script:

#!/bin/bash
# Search for similar callback patterns in the codebase
ast-grep --pattern 'const $_ = useCallback(($$$) => {
  dispatch($_);
  $$$
}, $_)'

# Find where tabClickHandler is defined
ast-grep --pattern 'const tabClickHandler = useCallback(($$$) => {
  $$$
}, $_)'

# Find other components using tabClickHandler
rg "tabClickHandler" -B 2 -A 2

Length of output: 59752

Copy link

Deploy-Preview-URL: https://ce-38272.dp.appsmith.com

alex-golovanov
alex-golovanov previously approved these changes Dec 20, 2024
ankitakinger
ankitakinger previously approved these changes Dec 20, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts (2)

79-88: Strengthen assertions with multiple checks

Add additional assertions to verify the complete state after tab navigation.

 cy.get("[data-testid='t--ide-tab-page2_js1']").click();

 jsEditor.currentJSObjectName().then((jsObjName) => {
   expect(jsObjName).equal("Page2_JS1");
+  // Add visibility check
+  cy.get("[data-testid='t--ide-tab-page2_js1']").should("have.class", "active");
+  // Verify other tab is not active
+  cy.get("[data-testid='t--ide-tab-page2_js2']").should("not.have.class", "active");
 });

19-148: Consider improving test isolation

While the test structure is good, consider:

  1. Adding cleanup in afterEach to ensure test isolation
  2. Using API calls to set up and tear down test data
  3. Implementing proper error handling for setup failures
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ac94099 and fae4d47.

📒 Files selected for processing (1)
  • app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts (1)

Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:

  • Follow best practices for Cypress code and e2e automation.
  • Avoid using cy.wait in code.
  • Avoid using cy.pause in code.
  • Avoid using agHelper.sleep().
  • Use locator variables for locators and do not use plain strings.
  • Use data-* attributes for selectors.
  • Avoid Xpaths, Attributes and CSS path.
  • Avoid selectors like .btn.submit or button[type=submit].
  • Perform logins via API with LoginFromAPI.
  • Perform logout via API with LogOutviaAPI.
  • Perform signup via API with SignupFromAPI.
  • Avoid using it.only.
  • Avoid using after and aftereach in test cases.
  • Use multiple assertions for expect statements.
  • Avoid using strings for assertions.
  • Do not use duplicate filenames even with different paths.
  • Avoid using agHelper.Sleep, this.Sleep in any file in code.

Comment on lines 20 to 23
before(() => {
datasources.CreateDataSource("Mongo");
cy.renameDatasource(dsName);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use API-based setup instead of UI interactions

Replace the UI-based datasource creation with API calls for better test reliability and performance.

- datasources.CreateDataSource("Mongo");
- cy.renameDatasource(dsName);
+ cy.request('POST', '/api/v1/datasources', {
+   name: dsName,
+   type: 'MongoDB',
+   // other required properties
+ });

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 34 to 44
cy.get("[data-testid='t--ide-tab-page1_js1']").click();

jsEditor.currentJSObjectName().then((jsObjName) => {
expect(jsObjName).equal("Page1_JS1");
});

cy.get("[data-testid='t--ide-tab-page1_js2']").click();

jsEditor.currentJSObjectName().then((jsObjName) => {
expect(jsObjName).equal("Page1_JS2");
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Extract tab selectors into locator variables

Move selectors to locator variables for better maintainability and reuse.

+ const tabLocators = {
+   js1Tab: "[data-testid='t--ide-tab-page1_js1']",
+   js2Tab: "[data-testid='t--ide-tab-page1_js2']"
+ };

- cy.get("[data-testid='t--ide-tab-page1_js1']").click();
+ cy.get(tabLocators.js1Tab).click();

- cy.get("[data-testid='t--ide-tab-page1_js2']").click();
+ cy.get(tabLocators.js2Tab).click();

jsEditor.CreateJSObject("", { prettify: false, toRun: false });
jsEditor.RenameJSObjFromPane("Page1_JS2");

cy.get("[data-testid='t--ide-tab-page1_js1']").click();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please agHelper.GetNClick(reconnectDatasourceModal.SkipToAppBtn,0,true, ); here for click function. And do follow for all.

Kindly add locator as variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
app/client/cypress/support/Pages/EditorNavigation.ts (1)

Line range hint 71-71: Replace Sleep() calls with proper Cypress wait commands.

Multiple methods use _.AggregateHelper.Sleep() which violates the coding guidelines. Consider replacing these with proper Cypress wait commands.

Replace the sleep calls with appropriate wait conditions:

 NavigateToWidget(name: string, clickOptions?: Partial<ClickOptions>, hierarchy: string[] = []) {
   AppSidebar.navigate(AppSidebarButton.Editor);
   PageLeftPane.switchSegment(PagePaneSegment.UI);
   hierarchy.forEach((level) => {
     PageLeftPane.expandCollapseItem(level);
   });
   PageLeftPane.selectItem(name, clickOptions);
-  _.AggregateHelper.Sleep();
+  cy.get('.t--entity-item.active').should('have.text', name);
 }

 NavigateToQuery(name: string) {
   AppSidebar.navigate(AppSidebarButton.Editor);
   PageLeftPane.switchSegment(PagePaneSegment.Queries);
   PageLeftPane.selectItem(name);
-  _.AggregateHelper.Sleep();
+  cy.get('.t--entity-item.active').should('have.text', name);
 }

 NavigateToJSObject(name: string) {
   AppSidebar.navigate(AppSidebarButton.Editor);
   PageLeftPane.switchSegment(PagePaneSegment.JS);
   PageLeftPane.selectItem(name);
-  _.AggregateHelper.Sleep();
+  cy.get('.t--entity-item.active').should('have.text', name);
 }

 NavigateToPage(name: string, networkCallAlias = false) {
   AppSidebar.navigate(AppSidebarButton.Editor);
   PageList.ShowList();
   PageLeftPane.selectItem(name, { multiple: true, force: true });
-  _.AggregateHelper.Sleep();
+  cy.get('.t--entity-item.active').should('have.text', name);
   networkCallAlias && _.AssertHelper.AssertNetworkStatus("pageSnap");
 }

Also applies to: 84-84, 91-91, 98-98

🧹 Nitpick comments (1)
app/client/cypress/support/Pages/EditorNavigation.ts (1)

23-25: Good implementation, consider adding input validation and documentation.

The selector follows best practices by using data-testid. However, consider these improvements:

+/**
+ * Generates a data-testid selector for IDE tabs
+ * @param name - The name of the tab
+ * @returns A data-testid selector string
+ * @throws {Error} If name is empty or undefined
+ */
 export const editorTabSelector = (name: string) => {
+  if (!name?.trim()) {
+    throw new Error('Tab name cannot be empty');
+  }
   return `[data-testid='t--ide-tab-${name.toLowerCase()}']`;
 };
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between fae4d47 and 3715ac2.

📒 Files selected for processing (2)
  • app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts (1 hunks)
  • app/client/cypress/support/Pages/EditorNavigation.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/cypress/e2e/Regression/ClientSide/IDE/Tabs_Navigation_spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/EditorNavigation.ts (1)

Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:

  • Follow best practices for Cypress code and e2e automation.
  • Avoid using cy.wait in code.
  • Avoid using cy.pause in code.
  • Avoid using agHelper.sleep().
  • Use locator variables for locators and do not use plain strings.
  • Use data-* attributes for selectors.
  • Avoid Xpaths, Attributes and CSS path.
  • Avoid selectors like .btn.submit or button[type=submit].
  • Perform logins via API with LoginFromAPI.
  • Perform logout via API with LogOutviaAPI.
  • Perform signup via API with SignupFromAPI.
  • Avoid using it.only.
  • Avoid using after and aftereach in test cases.
  • Use multiple assertions for expect statements.
  • Avoid using strings for assertions.
  • Do not use duplicate filenames even with different paths.
  • Avoid using agHelper.Sleep, this.Sleep in any file in code.

@hetunandu hetunandu merged commit 3d0de4f into release Dec 20, 2024
44 checks passed
@hetunandu hetunandu deleted the fix/tab-multipage-navigation-bug branch December 20, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Community Reported issues reported by community members IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product Medium Issues that frustrate users due to poor UX Needs Triaging Needs attention from maintainers to triage ok-to-test Required label for CI Production QA Pod Issues under the QA Pod QA Needs QA attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Navigation between Jsobjects not working properly
4 participants