chore: Changes for introducing disabled states for package pull in application #38222
Conversation
WalkthroughThis pull request introduces enhancements to package upgrade handling and UI interactions in the application. The changes primarily focus on adding infrastructure for managing package upgrade states, including a new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12389998439. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/ce/selectors/packageSelectors.ts (1)
20-21: Add JSDoc documentation for the selector functionAdd documentation to clarify this is a CE implementation and describe the expected behavior in EE.
+/** + * Selector to check if a package upgrade is in progress. + * Note: This is the CE implementation that always returns false. + * The EE implementation provides the actual package upgrade state. + */ export const getIsPackageUpgrading = (state: AppState): boolean => false;app/client/src/ce/components/BottomBar/GitActionsWrapper.tsx (1)
1-5: Enhance type safety and performanceConsider the following improvements:
- Define a proper interface for props
- Memoize the component to prevent unnecessary re-renders
- Add proper React import
+import React, { memo } from "react"; + +interface GitActionsWrapperProps { + children: React.ReactElement; +} + // This function is used to wrap the children in a disabled container if the package is upgrading // It's implemented in EE, but not in CE -function GitActionsWrapper({ children }: { children: React.ReactElement }) { +function GitActionsWrapper({ children }: GitActionsWrapperProps) { return children; } + +export default memo(GitActionsWrapper);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/ce/components/BottomBar/GitActionsWrapper.tsx(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/selectors/packageSelectors.ts(1 hunks)app/client/src/ee/components/BottomBar/GitActionsWrapper.tsx(1 hunks)app/client/src/pages/Editor/IDE/Header/index.tsx(4 hunks)app/client/src/pages/common/Disabler.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/components/BottomBar/GitActionsWrapper.tsx
🔇 Additional comments (6)
app/client/src/pages/common/Disabler.tsx (1)
15-15: LGTM: Appropriate cursor style for disabled state
The addition of cursor: not-allowed enhances the user experience by providing clear visual feedback.
app/client/src/ce/components/BottomBar/GitActionsWrapper.tsx (1)
1-7: Verify the implementation difference in EE
The comments indicate different behavior in EE. Let's verify the EE implementation to ensure consistency.
app/client/src/ce/constants/messages.ts (1)
480-481: LGTM! Clear and consistent message implementation.
The message provides good user feedback during package updates.
app/client/src/pages/Editor/IDE/Header/index.tsx (3)
83-83: LGTM! Well-documented styled component implementation.
The StyledTooltipTarget component is properly documented and solves the pointer-events issue for disabled button tooltips.
Also applies to: 91-96
141-141: LGTM! Clean implementation of package upgrading state.
The package upgrading state is properly integrated with existing protected mode check, and the tooltip text is appropriately dynamic.
Also applies to: 149-152
341-356: Verify the deploy button's disabled state behavior.
The deploy button's disabled state and tooltip implementation looks correct, but let's verify the integration.
✅ Verification successful
Let me verify the implementation of the protected mode integration.
Based on the search results, I can now verify the integration of the deploy button's disabled state. Let me check one more aspect of the implementation.
Deploy button's disabled state and tooltip implementation is correctly integrated.
The implementation properly handles:
- Disabled state through
isDeployDisabledcombining both package upgrading and protected mode states - Dynamic tooltip text that changes based on package upgrading status
- Proper button state management through the
isDisabledprop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the package upgrading selector integration and deploy button usage
# Check for other usages of package upgrading selector
rg "getIsPackageUpgrading"
# Check for deploy button disabled state usage
rg "isDeployDisabled"
Length of output: 616
Script:
#!/bin/bash
# Check for protected mode usage and its implementation
rg "isProtectedMode" -A 3
# Check for any other conditions that might affect deploy button state
ast-grep --pattern 'const isDeployDisabled = $_'
Length of output: 118636
Script:
# Check for tooltip text implementation
rg "deployTooltipText" -A 2
Length of output: 621
|
Deploy-Preview-URL: https://ce-38222.dp.appsmith.com |
ashit-rath
added
ok-to-test
Description
This PR introduces certain changes to facilitate disabled states when package pull is in progress.
EE PR -> #37841
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/12425304652
Commit: 3c3aed7
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 20 Dec 2024 04:56:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
GitActionsWrappercomponent for wrapping child elements.Headercomponent to manage deployment state based on package upgrades.Disablercomponent to visually indicate a disabled state.Bug Fixes