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

chore: Git mod - Connect/Import modal #38098

Merged
merged 5 commits into from
Dec 12, 2024
Merged

Conversation

ashit-rath
Copy link
Contributor

@ashit-rath ashit-rath commented Dec 11, 2024

Description

Git mod components, add connect/import from git modal components

Fixes #37812
Fixes #37802

Automation

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

🔍 Cypress test results

Tip

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


Thu, 12 Dec 2024 07:43:05 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a multi-step ConnectModal for Git provider connections.
    • Added components for generating SSH keys and managing Git remote URLs.
    • New constants for Git integration steps and demo GIFs for user guidance.
    • Added optional errorType property to enhance error handling in API responses.
    • New Steps component for step navigation in the modal.
    • New CopyButton component for clipboard functionality with visual feedback.
  • Improvements

    • Enhanced error handling and user prompts related to Git operations.
    • Improved user interface with styled components for better layout and presentation.
  • Bug Fixes

    • Improved validation and error messaging for SSH URL inputs.
  • Refactor

    • Renamed AutocommitStatusbar to Statusbar for consistency across components and tests.
  • Tests

    • Comprehensive test coverage for new components and functionalities related to Git integration.

@ashit-rath ashit-rath requested a review from brayn003 December 11, 2024 09:54
@ashit-rath ashit-rath self-assigned this Dec 11, 2024
Copy link
Contributor

coderabbitai bot commented Dec 11, 2024

Walkthrough

This pull request introduces several enhancements to the Git integration features within the application. Key changes include the addition of new components for managing SSH keys and selecting Git providers, as well as updates to existing error handling mechanisms. The APIResponseError interface now includes an optional errorType property, providing more context for errors. Additionally, multiple test files have been created to ensure the functionality of the new components and their interactions.

Changes

File Path Change Summary
app/client/src/api/ApiResponses.tsx Added optional property errorType to APIResponseError interface.
app/client/src/ce/constants/messages.ts Introduced new constants for Git integration and artifact management.
app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx Added unit tests for AddDeployKey component covering various functionalities.
app/client/src/git/components/ConnectModal/AddDeployKey.tsx New component for adding SSH deploy keys, includes error handling and UI elements.
app/client/src/git/components/ConnectModal/ChooseGitProvider.test.tsx Added unit tests for ChooseGitProvider component.
app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx New component for selecting a Git provider with state management.
app/client/src/git/components/ConnectModal/CopyButton.tsx New component for copying values to clipboard with visual feedback.
app/client/src/git/components/ConnectModal/GenerateSSH.test.tsx Added unit tests for GenerateSSH component.
app/client/src/git/components/ConnectModal/GenerateSSH.tsx New component for generating SSH keys and managing Git remote URLs.
app/client/src/git/components/ConnectModal/Steps.tsx New component for rendering step buttons in a horizontal layout.
app/client/src/git/components/ConnectModal/common.tsx New styled components for UI layout in the ConnectModal.
app/client/src/git/components/ConnectModal/constants.ts New constants for Git connection steps and demo GIFs.
app/client/src/git/components/ConnectModal/index.test.tsx Added comprehensive tests for ConnectModal component.
app/client/src/git/components/ConnectModal/index.tsx New multi-step modal component for connecting to Git providers.
app/client/src/git/components/Statusbar/index.test.tsx Refactored tests to rename AutocommitStatusbar to Statusbar.
app/client/src/git/components/Statusbar/index.tsx Renamed component from AutocommitStatusbar to Statusbar with updated props interface.
app/client/src/git/components/utils.ts Introduced GIT_REMOTE_URL_PATTERN regex and isValidGitRemoteUrl function for URL validation.
app/client/src/git/components/GitQuickActions/index.test.tsx Updated tests to reflect renaming of AutocommitStatusbar to Statusbar.
app/client/src/git/components/GitQuickActions/index.tsx Updated import and rendering logic to use Statusbar instead of AutocommitStatusbar.

Possibly related issues

Possibly related PRs

Suggested labels

Enhancement, Git Product, Git Platform

Suggested reviewers

  • ApekshaBhosale
  • ankitakinger
  • albinAppsmith

"In the realm of code where Git takes flight,
New components emerge, shining bright.
SSH keys and providers, all in a row,
With tests to ensure they function and flow.
Error types added, clarity in sight,
A pull request crafted, a developer's delight!" 🎉


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. (Experiment)
  • @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.

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: 6

🧹 Outside diff range and nitpick comments (23)
app/client/src/git/components/Statusbar/index.tsx (2)

5-10: Consider using discriminated union type for props

The props interface could be more type-safe by using a discriminated union type to handle the completed/incomplete states differently.

-interface StatusbarProps {
-  message?: string;
-  completed: boolean;
-  onHide?: () => void;
-  className?: string;
-  testId?: string;
+type StatusbarProps =
+  | {
+      completed: false;
+      message?: string;
+      className?: string;
+      testId?: string;
+    }
+  | {
+      completed: true;
+      message?: string;
+      onHide: () => void;
+      className?: string;
+      testId?: string;
+    };

Line range hint 109-116: Unused props not being passed to ADSStatusBar

The className and testId props are defined but not utilized in the component.

-    <StatusbarWrapper data-testid="t--git-statusbar">
+    <StatusbarWrapper data-testid={testId || "t--git-statusbar"} className={className}>
app/client/src/git/components/Statusbar/index.test.tsx (1)

Line range hint 20-155: Add test cases for new props

The test suite needs to be extended to cover the new message, className, and testId props.

Add these test cases:

it("should render with custom message", () => {
  const message = "Custom Message";
  render(<Statusbar completed={false} message={message} />);
  expect(screen.getByText(message)).toBeInTheDocument();
});

it("should apply custom className and testId", () => {
  const className = "custom-class";
  const testId = "custom-test-id";
  render(<Statusbar completed={false} className={className} testId={testId} />);
  const wrapper = screen.getByTestId(testId);
  expect(wrapper).toHaveClass(className);
});
app/client/src/git/components/QuickActions/index.tsx (2)

131-134: Simplify Statusbar props

The completed prop is being set to the negation of isPollingAutocommit, which makes the code less intuitive.

-        <Statusbar
-          completed={!isPollingAutocommit}
-          message={createMessage(AUTOCOMMIT_IN_PROGRESS_MESSAGE)}
-        />
+        <Statusbar
+          completed={false}
+          message={createMessage(AUTOCOMMIT_IN_PROGRESS_MESSAGE)}
+        />

Line range hint 67-71: Remove redundant type assertion

The isAutocommitEnabled variable has an explicit type annotation that matches its inferred type.

-  // TODO - Update once the gitMetadata typing is added
-  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-  // @ts-ignore
-  const isAutocommitEnabled: boolean = gitMetadata?.autoCommitConfig?.enabled;
+  const isAutocommitEnabled = gitMetadata?.autoCommitConfig?.enabled ?? false;
app/client/src/git/components/QuickActions/index.test.tsx (1)

19-20: Update test ID to match renamed component

The test ID still uses "autocommit-statusbar" while the component has been renamed to "Statusbar". Consider updating for consistency:

jest.mock("./../Statusbar", () => () => (
-  <div data-testid="autocommit-statusbar">Statusbar</div>
+  <div data-testid="statusbar">Statusbar</div>
));
app/client/src/git/components/ConnectModal/AddDeployKey.tsx (1)

207-207: Simplify conditional using optional chaining

You can simplify the conditional statement by utilizing optional chaining.

Apply this diff:

- if (sshKeyPair && sshKeyPair.includes("rsa")) {
+ if (sshKeyPair?.includes("rsa")) {
🧰 Tools
🪛 Biome (1.9.4)

[error] 207-207: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

app/client/src/git/components/ConnectModal/index.tsx (1)

95-100: Remove unnecessary comments

The comments referencing replaced code may no longer be necessary. Consider removing them to keep the code clean.

Apply this diff:

app/client/src/git/components/utils.ts (2)

1-12: Consider using a library for Git URL validation

Using a custom regex for validating Git remote URLs can be error-prone. Consider leveraging a well-tested library or module to handle Git URL parsing and validation for improved reliability.


11-12: Add unit tests for 'isValidGitRemoteUrl' function

To ensure robustness, add comprehensive unit tests covering various valid and invalid Git remote URLs.

Would you like assistance in creating unit tests for this function?

app/client/src/git/components/ConnectModal/common.tsx (1)

42-48: Consider responsive height for DemoImage.

The fixed height of 300px might not be optimal for all screen sizes. Consider using relative units or min/max height constraints.

-  height: 300px;
+  min-height: 200px;
+  max-height: 400px;
+  height: 40vh;
app/client/src/git/components/ConnectModal/Steps.tsx (2)

106-106: Remove redundant role attribute

The role="button" attribute is unnecessary as it's already a button element.

-  role="button"

90-94: Optimize onClick handler creation

Move the handler creation outside the render loop to prevent recreating it for each step.

-  const onClick = (step: Step, index: number) => () => {
-    if (index < activeIndex) {
-      onActiveKeyChange(step.key);
-    }
-  };
+  const onClick = useCallback((step: Step, index: number) => {
+    if (index < activeIndex) {
+      onActiveKeyChange(step.key);
+    }
+  }, [activeIndex, onActiveKeyChange]);
app/client/src/git/components/ConnectModal/GenerateSSH.tsx (2)

71-81: Remove TODO comment and implement feature flag

The comment indicates temporary hardcoding of error messages. This should be properly implemented with feature flags.

Would you like me to help implement the feature flag integration for these error messages?


56-59: Simplify validation logic

The validation condition can be simplified for better readability.

-  const isInvalid =
-    isTouched &&
-    (typeof value?.remoteUrl !== "string" ||
-      !isValidGitRemoteUrl(value?.remoteUrl));
+  const isInvalid = isTouched && !isValidGitRemoteUrl(value?.remoteUrl ?? '');
app/client/src/git/components/ConnectModal/GenerateSSH.test.tsx (2)

91-105: Add edge cases for URL validation

The URL validation tests should include more edge cases.

Add test cases for:

  • Empty string
  • URL with special characters
  • URL with different SSH formats
it.each([
  ['', false],
  ['[email protected]:user/repo.git', true],
  ['ssh://[email protected]:user/repo.git', true],
  ['https://github.com/user/repo.git', false],
])('validates URL %s correctly', (url, expected) => {
  // Test implementation
});

13-19: Use test data constants

Extract test data to constants for reusability across tests.

+const TEST_DATA = {
+  VALID_URL: '[email protected]:user/repo.git',
+  INVALID_URL: 'invalid-url',
+  DEFAULT_PROVIDER: 'github' as GitProvider,
+};
+
 const defaultProps = {
   onChange: jest.fn(),
   value: {
-    gitProvider: "github" as GitProvider,
+    gitProvider: TEST_DATA.DEFAULT_PROVIDER,
     remoteUrl: "",
   },
 };
app/client/src/git/components/ConnectModal/index.test.tsx (2)

34-34: Consider improving test descriptions for clarity

The test descriptions could be more specific about the scenarios being tested. For example:

  • "renders the initial step" -> "renders the initial ChooseGitProvider step with disabled next button"
  • "renders the component and initial fields" -> "renders all Git provider radio options in initial state"

Also applies to: 66-66, 76-76


161-165: Consider testing more error scenarios

The error testing is limited to one specific error code (AE-GIT-4033). Consider adding tests for other potential error scenarios.

app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (2)

46-46: Consider using an enum for Git providers

Using a const array with as const is good, but an enum might provide better type safety and IDE support.

-const GIT_PROVIDERS = ["github", "gitlab", "bitbucket", "others"] as const;
+enum GitProvider {
+  GITHUB = "github",
+  GITLAB = "gitlab",
+  BITBUCKET = "bitbucket",
+  OTHERS = "others"
+}

126-149: Add aria-label to radio buttons for better accessibility

The radio buttons should have descriptive aria-labels for screen readers.

 <Radio
   data-testid="t--git-provider-radio-github"
+  aria-label="Choose GitHub as Git provider"
   value="github"
 >
   Github
 </Radio>
app/client/src/git/components/ConnectModal/ChooseGitProvider.test.tsx (2)

213-224: Consider testing mobile device conditions

While the test covers canCreateNewArtifact, it should also test the mobile device condition since the component uses useIsMobileDevice.

Add a test that mocks useIsMobileDevice to return true and verifies the expected behavior.


1-1: Remove eslint-disable comment

Instead of disabling the rule globally, consider fixing the underlying issue or using more specific disabling.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 83db020 and a060095.

📒 Files selected for processing (19)
  • app/client/src/api/ApiResponses.tsx (1 hunks)
  • app/client/src/ce/constants/messages.ts (1 hunks)
  • app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/AddDeployKey.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/CopyButton.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/GenerateSSH.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/GenerateSSH.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/Steps.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/common.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/constants.ts (1 hunks)
  • app/client/src/git/components/ConnectModal/index.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/index.tsx (1 hunks)
  • app/client/src/git/components/QuickActions/index.test.tsx (2 hunks)
  • app/client/src/git/components/QuickActions/index.tsx (3 hunks)
  • app/client/src/git/components/Statusbar/index.test.tsx (9 hunks)
  • app/client/src/git/components/Statusbar/index.tsx (4 hunks)
  • app/client/src/git/components/utils.ts (1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
  • app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx
  • app/client/src/ce/constants/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx

[error] 244-244: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

app/client/src/git/components/ConnectModal/AddDeployKey.tsx

[error] 207-207: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
app/client/src/api/ApiResponses.tsx (1)

4-4: Addition of 'errorType' property is acceptable

Adding the optional errorType property to APIResponseError enhances error handling and maintains backward compatibility.

app/client/src/git/components/ConnectModal/constants.ts (2)

4-8: LGTM! Well-structured step constants.

The Git connection steps are clearly defined and follow good naming conventions.


10-26: Consider environment-specific CDN URL configuration.

While the current implementation works, consider making the CDN URLs configurable based on the environment to support different deployment scenarios.

@github-actions github-actions bot added Packages & Git Pod All issues belonging to Packages and Git Packages Pod issues that belong to the packages pod Task A simple Todo skip-changelog Adding this label to a PR prevents it from being listed in the changelog labels Dec 11, 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: 1

🧹 Outside diff range and nitpick comments (1)
app/client/src/ce/constants/messages.ts (1)

1072-1075: LGTM! Well-structured message constants.

The new constants follow the established patterns and provide good reusability for different artifact types.

Consider caching the lowercase artifactType to avoid repeated calls to toLocaleLowerCase():

export const IMPORT_ARTIFACT_IF_NOT_EMPTY = (artifactType: string) => {
+  const lowerType = artifactType.toLocaleLowerCase();
-  `If you already have an ${artifactType.toLocaleLowerCase()} connected to Git, you can import it to the workspace.`;
+  return `If you already have an ${lowerType} connected to Git, you can import it to the workspace.`;
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a060095 and 7cb8ef5.

📒 Files selected for processing (3)
  • app/client/src/ce/constants/messages.ts (1 hunks)
  • app/client/src/git/components/GitQuickActions/index.test.tsx (2 hunks)
  • app/client/src/git/components/GitQuickActions/index.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • app/client/src/git/components/GitQuickActions/index.tsx
🔇 Additional comments (1)
app/client/src/git/components/GitQuickActions/index.test.tsx (1)

Line range hint 82-98: LGTM! Test case updated correctly

The test case name and implementation have been updated to reflect the component rename from AutocommitStatusbar to Statusbar while maintaining the same test logic and assertions.

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: 3

🧹 Outside diff range and nitpick comments (5)
app/client/src/git/components/ConnectModal/CopyButton.tsx (3)

9-13: Add type safety to styled components

Consider extending HTML element interfaces for better type safety and prop validation.

-export const TooltipWrapper = styled.div`
+export const TooltipWrapper = styled.div<{ $customProp?: string }>`

17-25: Add JSDoc comments to interface properties

Document the purpose and constraints of each prop for better maintainability.

 interface CopyButtonProps {
+  /** Custom styles to apply to the container */
   style?: CSSProperties;
+  /** The text value to copy to clipboard */
   value?: string;
+  /** Duration in milliseconds to show the success state */
   delay?: number;
+  /** Callback function triggered after successful copy */
   onCopy?: () => void;
+  /** Message to display in the tooltip */
   tooltipMessage?: string;
+  /** Whether the button is disabled */
   isDisabled?: boolean;
+  /** Suffix for test IDs */
   testIdSuffix?: string;
 }

70-97: Optimize rendering performance

Consider memoizing the rendered content to prevent unnecessary re-renders.

+  const renderCopyButton = useMemo(() => (
+    <TooltipWrapper style={style}>
+      <Tooltip content={tooltipMessage}>
+        <Button
+          aria-label={`Copy ${tooltipMessage || "text"}`}
+          data-testid={`t--copy-${testIdSuffix}`}
+          isDisabled={isDisabled}
+          isIconButton
+          kind="tertiary"
+          onClick={copyToClipboard}
+          size="sm"
+          startIcon="duplicate"
+        />
+      </Tooltip>
+    </TooltipWrapper>
+  ), [style, tooltipMessage, isDisabled, testIdSuffix, copyToClipboard]);

   return (
     <>
-      {showCopied ? (
+      {showCopied ? ( 
         <IconContainer style={style}>
           <Icon
             color="var(--ads-v2-color-fg-success)"
             name="check-line"
             size="lg"
           />
         </IconContainer>
-      ) : (
-        <TooltipWrapper style={style}>
-          <Tooltip content={tooltipMessage}>
-            <Button
-              aria-label={`Copy ${tooltipMessage || "text"}`}
-              data-testid={`t--copy-${testIdSuffix}`}
-              isDisabled={isDisabled}
-              isIconButton
-              kind="tertiary"
-              onClick={copyToClipboard}
-              size="sm"
-              startIcon="duplicate"
-            />
-          </Tooltip>
-        </TooltipWrapper>
-      )}
+      ) : renderCopyButton}
     </>
   );
app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (2)

46-48: Consider using enum for Git providers

The current implementation with const assertion is good, but an enum would provide better type safety and maintainability.

-const GIT_PROVIDERS = ["github", "gitlab", "bitbucket", "others"] as const;
-export type GitProvider = (typeof GIT_PROVIDERS)[number];
+export enum GitProvider {
+  GITHUB = "github",
+  GITLAB = "gitlab",
+  BITBUCKET = "bitbucket",
+  OTHERS = "others"
+}

185-190: Add loading state for demo images

The DemoImage component should handle loading states to prevent layout shifts.

 <DemoImage
   alt={`Create an empty repo in ${value?.gitProvider}`}
+  loading="lazy"
   src={GIT_DEMO_GIF.create_repo[value?.gitProvider || "github"]}
 />
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7cb8ef5 and 8d77b16.

📒 Files selected for processing (3)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/CopyButton.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.test.tsx
🔇 Additional comments (1)
app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (1)

27-35: Verify all required message constants

Let's ensure all message constants are properly defined.

✅ Verification successful

All message constants are properly defined

The imported message constants are correctly defined in the CE (Community Edition) messages file which is re-exported by the EE (Enterprise Edition) messages file. The constants are used for Git provider selection and repository connection flows.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all required message constants are defined

# Search for message constants in the messages file
rg "CHOOSE_A_GIT_PROVIDER_STEP|CHOOSE_GIT_PROVIDER_QUESTION|HOW_TO_CREATE_EMPTY_REPO|IMPORT_ARTIFACT_IF_NOT_EMPTY|IS_EMPTY_REPO_QUESTION|I_HAVE_EXISTING_ARTIFACT_REPO|NEED_EMPTY_REPO_MESSAGE" "ee/constants/messages.ts"

Length of output: 283


Script:

#!/bin/bash
# First, let's find the correct location of the messages file
fd "messages.ts" --type f

# Then, let's check the content of any messages.ts files found
fd "messages.ts" --type f --exec cat {}

Length of output: 68479


Script:

#!/bin/bash
# Let's check the content of the specific messages file
cat app/client/src/ee/constants/messages.ts

Length of output: 84


Script:

#!/bin/bash
# Let's check the content of the CE messages file since EE exports from it
cat app/client/src/ce/constants/messages.ts

Length of output: 125183

@ashit-rath ashit-rath added the ok-to-test Required label for CI label Dec 11, 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: 0

🧹 Outside diff range and nitpick comments (7)
app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (3)

47-49: Consider using TypeScript enum for Git providers

Convert the constant array to an enum for better type safety and maintainability.

-const GIT_PROVIDERS = ["github", "gitlab", "bitbucket", "others"] as const;
-export type GitProvider = (typeof GIT_PROVIDERS)[number];
+export enum GitProvider {
+  GITHUB = "github",
+  GITLAB = "gitlab",
+  BITBUCKET = "bitbucket",
+  OTHERS = "others"
+}
+
+const GIT_PROVIDERS = Object.values(GitProvider);

80-93: Enhance error handling in onGitProviderChange

Consider throwing a custom error or handling the error case more gracefully.

 const onGitProviderChange = useCallback(
   (value: string) => {
     const gitProvider = GIT_PROVIDERS.includes(value as GitProvider)
       ? (value as GitProvider)
       : undefined;

     if (gitProvider) {
       onChange({ gitProvider });
     } else {
-      log.error(`Invalid git provider: ${value}`);
+      log.error(new Error(`Invalid git provider: ${value}`));
+      // Consider showing user feedback
+      onChange({ gitProvider: undefined });
     }
   },
   [onChange],
 );

190-195: Add fallback for undefined Git provider in alt text

The alt text should handle the case when gitProvider is undefined.

-alt={`Create an empty repo in ${value?.gitProvider}`}
+alt={`Create an empty repo in ${value?.gitProvider || 'selected provider'}`}
app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx (4)

8-15: Consider enhancing mock implementations for better type safety.

The mocks could be more explicit about their types and return values.

 jest.mock("ee/utils/AnalyticsUtil", () => ({
-  logEvent: jest.fn(),
+  logEvent: jest.fn<void, [string, ...unknown[]]>(),
 }));

 jest.mock("copy-to-clipboard", () => ({
   __esModule: true,
-  default: () => true,
+  default: jest.fn<boolean, [string]>().mockReturnValue(true),
 }));

32-32: Consider making the truncated SSH key more obvious in test data.

-  sshKeyPair: "ecdsa-sha2-nistp256 AAAAE2VjZHNhAAAIBaj...",
+  sshKeyPair: "ecdsa-sha2-nistp256 AAAAE2VjZHNhAAAIBaj[...truncated for tests]",

248-250: Simplify callback handling with optional chaining.

-    const fetchSSHKeyPair = jest.fn((props) => {
-      props.onSuccessCallback && props.onSuccessCallback();
-    });
+    const fetchSSHKeyPair = jest.fn((props) => {
+      props.onSuccessCallback?.();
+    });
🧰 Tools
🪛 Biome (1.9.4)

[error] 249-249: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


247-269: Consider adding test coverage for SSH key generation failure.

The current tests verify successful SSH key generation, but there's no coverage for the failure scenario.

Add a test case like this:

it("handles SSH key generation failure gracefully", async () => {
  const generateSSHKey = jest.fn().mockRejectedValue(new Error("Generation failed"));
  
  render(
    <AddDeployKey
      {...defaultProps}
      generateSSHKey={generateSSHKey}
      sshKeyPair=""
    />
  );

  await waitFor(() => {
    expect(generateSSHKey).toHaveBeenCalled();
    // Add assertions for error state/message
  });
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 249-249: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8d77b16 and e94ebe0.

📒 Files selected for processing (3)
  • app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (1 hunks)
  • app/client/src/git/components/ConnectModal/CopyButton.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/git/components/ConnectModal/CopyButton.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/git/components/ConnectModal/AddDeployKey.test.tsx

[error] 249-249: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (2)
app/client/src/git/components/ConnectModal/ChooseGitProvider.tsx (2)

126-155: Add ARIA label for better accessibility

The RadioGroup should have an aria-label for screen readers.


233-233: LGTM!

The export statement is correctly implemented.

@brayn003 brayn003 merged commit 5262438 into release Dec 12, 2024
44 checks passed
@brayn003 brayn003 deleted the chore/git-mod-connect-modal branch December 12, 2024 11:04
@coderabbitai coderabbitai bot mentioned this pull request Jan 13, 2025
2 tasks
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Feb 7, 2025
## Description
Git mod components, add connect/import from git modal components

Fixes appsmithorg#37812
Fixes appsmithorg#37802

## Automation

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

### 🔍 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/12291098002>
> Commit: e94ebe0
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12291098002&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Git`
> Spec:
> <hr>Thu, 12 Dec 2024 07:43:05 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
  - Introduced a multi-step `ConnectModal` for Git provider connections.
- Added components for generating SSH keys and managing Git remote URLs.
- New constants for Git integration steps and demo GIFs for user
guidance.
- Added optional `errorType` property to enhance error handling in API
responses.
  - New `Steps` component for step navigation in the modal.
- New `CopyButton` component for clipboard functionality with visual
feedback.

- **Improvements**
  - Enhanced error handling and user prompts related to Git operations.
- Improved user interface with styled components for better layout and
presentation.

- **Bug Fixes**
  - Improved validation and error messaging for SSH URL inputs.

- **Refactor**
- Renamed `AutocommitStatusbar` to `Statusbar` for consistency across
components and tests.

- **Tests**
- Comprehensive test coverage for new components and functionalities
related to Git integration.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Required label for CI Packages & Git Pod All issues belonging to Packages and Git Packages Pod issues that belong to the packages pod skip-changelog Adding this label to a PR prevents it from being listed in the changelog Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Git mod - Components - Git import [Task] Git mod - Components - Connect modal
2 participants