fix: git merge button visibility on branch change after a successful merge #38847
Conversation
WalkthroughThis pull request introduces a mechanism to reset the merge success state across multiple components in the Git integration module. The changes span four files, adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
Bug
![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 (1)
app/client/src/git/components/OpsModal/TabMerge/TabMergeView.tsx (1)
211-222: Consider consolidating merge state management.The component manages multiple merge-related states (
mergeStatus,isMergeSuccess, etc.). Consider extracting this logic into a custom hook for better maintainability.This would help:
- Centralize merge state management
- Simplify the component logic
- Make state transitions more predictable
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/src/git/components/OpsModal/TabMerge/TabMergeView.tsx(3 hunks)app/client/src/git/components/OpsModal/TabMerge/index.tsx(2 hunks)app/client/src/git/hooks/useMerge.ts(2 hunks)app/client/src/git/store/actions/mergeActions.ts(1 hunks)app/client/src/git/store/gitArtifactSlice.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (7)
app/client/src/git/store/actions/mergeActions.ts (1)
44-48: LGTM! Clean implementation of the reset action.The action follows the established pattern and maintains a single responsibility of resetting the merge success state.
app/client/src/git/components/OpsModal/TabMerge/index.tsx (1)
19-19: LGTM! Clean prop handling.The
resetMergeSuccessStateis properly extracted from the hook and passed down to the child component.Also applies to: 46-46
app/client/src/git/hooks/useMerge.ts (1)
69-73: LGTM! Well-structured hook implementation.The
resetMergeSuccessStatefunction is properly memoized and follows the established pattern for state management functions.Also applies to: 86-86
app/client/src/git/store/gitArtifactSlice.ts (1)
88-88: LGTM! Proper Redux integration.The action is correctly imported and integrated into the reducers object in the appropriate section.
Also applies to: 196-196
app/client/src/git/components/OpsModal/TabMerge/TabMergeView.tsx (3)
83-83: LGTM! Interface update is clean and well-typed.The addition of
resetMergeSuccessStateto the interface maintains type safety.
103-103: LGTM! Default prop value prevents runtime errors.Good practice using
noopas the default value to prevent undefined function calls.
213-221: Verify the timing of merge status reset.The implementation correctly resets the merge success state when selecting a new branch. However, we should verify that this doesn't interfere with the merge status fetching.
Let's verify the sequence of operations:
✅ Verification successful
The merge status reset implementation is safe and well-structured.
The reset occurs after initiating the fetch but doesn't interfere with its completion. The Redux store properly manages state transitions, and the UI correctly reflects all possible states including loading states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might be affected by the merge success state reset rg "resetMergeSuccessState" -B 2 -A 2 # Look for potential race conditions in merge status related effects ast-grep --pattern 'useEffect($$$fetchMergeStatus$$$)'Length of output: 69556
Script:
#!/bin/bash # Find the fetchMergeStatus implementation and its dependencies ast-grep --pattern 'const fetchMergeStatus = $$$' # Look for components using mergeSuccess state rg "mergeSuccess" -B 2 -A 2 # Find other useEffect hooks that might be affected by merge status changes ast-grep --pattern 'useEffect($$$mergeStatus$$$)'Length of output: 77676
Script:
#!/bin/bash # Find the Redux reducer handling merge status rg "case ['\"].*MERGE.*['\"]" -B 2 -A 4 # Find components subscribing to merge status state rg "useSelector.*merge" -B 2 -A 2 # Find the complete chain of merge status actions rg "MERGE_STATUS" -B 2 -A 2Length of output: 15794
…merge (appsmithorg#38847) ## Description The `mergeSuccess` status in the reducer was not getting reset on branch change. Introduced a new action to reset it on change Fixes appsmithorg#38844 ## 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/12983463132> > Commit: f012fbb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12983463132&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Mon, 27 Jan 2025 07:39:33 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** - Enhanced merge state management with a new `resetMergeSuccessState` function - Improved ability to reset merge success state during branch selection - **Bug Fixes** - Resolved potential state persistence issues in merge operations - **Refactor** - Updated hooks and components to support more flexible merge state handling <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
The
mergeSuccessstatus in the reducer was not getting reset on branch change.Introduced a new action to reset it on change
Fixes #38844
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/12983463132
Commit: f012fbb
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 27 Jan 2025 07:39:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
resetMergeSuccessStatefunctionBug Fixes
Refactor