chore: git modularisation disconnect modal#37938
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/client/src/git/components/DisconnectModal/index.tsx (3)
26-34: Consider moving documentation URL to constants and adding type safetyThe documentation URL should be moved to a constants file for better maintainability, and the link props should be typed.
+interface DocsLinkProps { + children: string; + to: string; + className: string; +} -const DOCS_LINK_PROPS = [ +const DOCS_LINK_PROPS: DocsLinkProps[] = [
67-70: Consider case-insensitive comparison for app name validationThe strict equality comparison might frustrate users if they match the name but with different casing.
- appName !== disconnectingApp.name || + appName.toLowerCase() !== disconnectingApp.name.toLowerCase() ||
109-116: Enhance accessibility with ARIA attributesConsider adding aria-label and aria-describedby to improve screen reader support.
<Input className="t--git-app-name-input" label={createMessage(APPLICATION_NAME)} onBlur={inputOnBlur} onChange={inputOnChange} size="md" value={appName} + aria-label="Confirm application name for disconnection" + aria-describedby="disconnect-warning" />app/client/src/git/components/DisconnectModal/index.test.tsx (2)
27-105: Add tests for edge casesConsider adding tests for:
- Modal close behavior when clicking outside
- Error handling for empty app ID
- Maximum length validation for input
106-165: Add async behavior testsConsider adding tests for:
- Loading states during disconnect operation
- Error handling during disconnect
- Retry scenarios
it('should show loading state during disconnect', async () => { // Test implementation });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/git/components/DisconnectModal/index.test.tsx(1 hunks)app/client/src/git/components/DisconnectModal/index.tsx(1 hunks)
🔇 Additional comments (3)
app/client/src/git/components/DisconnectModal/index.tsx (2)
37-51: LGTM! Clean props interface and styling
81-89: Review analytics data exposure
The analytics event logs both the user input and expected app name. Consider hashing or omitting sensitive data.
app/client/src/git/components/DisconnectModal/index.test.tsx (1)
1-26: LGTM! Well-structured test setup with proper mocks
## Description Decouple and move disconnect modal Fixes appsmithorg#37811 ## 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/12155733900> > Commit: f30ad80 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12155733900&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Wed, 04 Dec 2024 09:17:09 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 - **New Features** - Introduced the `DisconnectModal` component for disconnecting Git repositories from applications. - Added input validation for the application name with a "Revoke" button that is conditionally enabled. - Included a warning message about the irreversible action and a link to documentation. - **Tests** - Implemented a comprehensive test suite for the `DisconnectModal` to ensure correct functionality and user interactions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Decouple and move disconnect modal
Fixes #37811
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/12155733900
Commit: f30ad80
Cypress dashboard.
Tags:
@tag.GitSpec:
Wed, 04 Dec 2024 09:17:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
DisconnectModalcomponent for disconnecting Git repositories from applications.Tests
DisconnectModalto ensure correct functionality and user interactions.