chore: git mod - minor fixes #38563
chore: git mod - minor fixes #38563
Conversation
WalkthroughThis pull request introduces significant changes to the Git synchronization and status tracking mechanisms in the application. The modifications primarily focus on transforming how module instances and source modules are processed, removing package-related tracking, and updating various components to reflect these changes. The updates span multiple files, affecting status transformations, component props, hooks, and selectors to align with a new approach to tracking Git changes. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GitStatusTransformer
participant StatusSelector
participant UIComponent
Client->>GitStatusTransformer: Fetch Git Status
GitStatusTransformer->>StatusSelector: Transform Status
StatusSelector-->>UIComponent: Update with Source Modules
UIComponent->>Client: Render Changes
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
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/client/src/pages/Editor/gitSync/components/GitChangesList/StaticChange.tsx (1)
59-62: Simplify the conditional message formatting.The conditional string formatting can be simplified using template literals.
- message: `${status.modifiedSourceModules ?? 0} ${ - (status.modifiedSourceModules ?? 0) > 0 ? "modules" : "module" - } modified`, + message: `${status.modifiedSourceModules ?? 0} module${status.modifiedSourceModules !== 1 ? 's' : ''} modified`,app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts (1)
275-276: Add type safety for status parameters.Consider adding explicit type annotations for the destructured values to ensure type safety.
- const { modifiedModuleInstances = 0, modifiedSourceModules = 0 } = status; + const { modifiedModuleInstances = 0 as number, modifiedSourceModules = 0 as number } = status;app/client/src/selectors/gitSyncSelectors.tsx (1)
Line range hint
149-167: Consider grouping related modifications.The count calculation could be more maintainable by grouping related modifications.
- return ( - modifiedDatasources + - modifiedJSLibs + - modifiedJSObjects + - modifiedSourceModules + - modifiedModuleInstances + - modifiedPages + - modifiedQueries + - themeCount + - settingsCount - ); + const moduleChanges = modifiedSourceModules + modifiedModuleInstances; + const jsChanges = modifiedJSLibs + modifiedJSObjects; + const pageChanges = modifiedPages + modifiedQueries; + const configChanges = themeCount + settingsCount; + + return moduleChanges + jsChanges + pageChanges + configChanges + modifiedDatasources;app/client/src/reducers/uiReducers/gitSyncReducer.ts (1)
757-758: Consider using strict types for module counts.The changes look good. However, since these represent counts, consider using non-nullable types with default values.
- modifiedSourceModules?: number; - modifiedModuleInstances?: number; + modifiedSourceModules: number = 0; + modifiedModuleInstances: number = 0;app/client/src/git/hooks/useDiscard.ts (1)
9-10: Consider adding type safety for artifact ID.The changes look good and improve artifact handling. Consider adding type assertion or validation for the artifact ID.
- const artifactId = artifact?.id; + const artifactId = artifact?.id as string | undefined; + if (!artifactId) { + console.warn('Artifact ID is missing'); + }Also applies to: 16-17, 19-19
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts(2 hunks)app/client/src/git/components/QuickActions/BranchButton.tsx(1 hunks)app/client/src/git/components/QuickActions/QuickActionsView.tsx(1 hunks)app/client/src/git/components/QuickActions/hooks/useStatusChangeCount.ts(2 hunks)app/client/src/git/components/QuickActions/index.tsx(1 hunks)app/client/src/git/hooks/useDiscard.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.types.ts(1 hunks)app/client/src/git/sagas/discardSaga.ts(1 hunks)app/client/src/git/store/actions/discardActions.ts(1 hunks)app/client/src/pages/Editor/gitSync/components/GitChangesList/StaticChange.tsx(1 hunks)app/client/src/pages/Editor/gitSync/components/GitChangesList/index.test.tsx(1 hunks)app/client/src/pages/Editor/gitSync/components/GitChangesList/index.tsx(0 hunks)app/client/src/reducers/uiReducers/gitSyncReducer.ts(1 hunks)app/client/src/selectors/gitModSelectors.ts(1 hunks)app/client/src/selectors/gitSyncSelectors.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/pages/Editor/gitSync/components/GitChangesList/index.tsx
🧰 Additional context used
📓 Learnings (1)
app/client/src/git/components/QuickActions/QuickActionsView.tsx (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38060
File: app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts:16-24
Timestamp: 2024-12-10T10:53:17.146Z
Learning: In the `getPullBtnStatus` function (`app/client/src/git/components/GitQuickActions/helpers/getPullButtonStatus.ts`), default parameter values should be explicitly mentioned to handle component state properly, even if all props are required.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (12)
app/client/src/git/store/actions/discardActions.ts (2)
7-9: LGTM! Good type safety improvement.The new
DiscardInitPayloadinterface properly extendsGitArtifactBasePayloadand adds the requiredartifactIdfield.
11-18: LGTM! Clean action creator implementation.The action creator maintains immutability and follows Redux best practices.
app/client/src/git/requests/fetchStatusRequest.types.ts (1)
29-30: LGTM! Consistent type definitions.The new
modifiedSourceModulesfield aligns with the module tracking improvements and maintains consistency with other count fields.app/client/src/git/components/QuickActions/hooks/useStatusChangeCount.ts (2)
11-11: LGTM! Proper status calculation update.The new module tracking fields are correctly incorporated into the status calculation.
Also applies to: 14-14
27-28: Verify the order of operations.Ensure that adding
modifiedSourceModulesandmodifiedModuleInstancesin this order aligns with the expected priority in the total count.app/client/src/git/sagas/discardSaga.ts (2)
14-17: LGTM! Proper type safety in saga.The saga correctly uses the new
DiscardInitPayloadtype and properly destructures the required fields.
22-22: Verify error handling for artifactId.Ensure that
artifactIdis validated before making the request call.app/client/src/selectors/gitModSelectors.ts (1)
24-25: Verify the impact of removing feature flag.The change forces Git modularization to be always enabled by removing the feature flag check. This could impact users if the feature isn't fully tested.
Run this script to check for any feature flag dependencies:
✅ Verification successful
Feature flag removal appears to be a planned graduation
The change is safe as the feature was already enabled in test environments and appears to be a planned graduation from feature flag to permanent feature. The implementation is well-integrated across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of the git modularisation feature flag rg -l "release_git_modularisation_enabled"Length of output: 179
Script:
#!/bin/bash # Check the feature flag definition and usage echo "=== Feature Flag Definition ===" cat app/client/src/ce/entities/FeatureFlag.ts echo -e "\n=== Cypress Test Usage ===" cat app/client/cypress/support/Objects/FeatureFlags.ts echo -e "\n=== Searching for selectGitModEnabled usage ===" rg "selectGitModEnabled" -A 2Length of output: 11539
app/client/src/pages/Editor/gitSync/components/GitChangesList/index.test.tsx (1)
348-348: LGTM! Test updated to match new data structure.The test case correctly reflects the change from
modifiedModulestomodifiedSourceModules.app/client/src/git/components/QuickActions/BranchButton.tsx (1)
54-54: LGTM - Consistent with parent componentThe default value change maintains consistency with the parent component's behavior.
app/client/src/git/components/QuickActions/QuickActionsView.tsx (2)
67-67: LGTM - Consistent default valueThe default value change maintains consistency across the component hierarchy.
Line range hint
82-86: Verify pull button behavior with undefined statusThe pull button status depends on
isStatusClean. With the new default value, verify that the pull button behaves correctly when the Git status is undefined.✅ Verification successful
Pull button behavior with undefined status is properly handled
The code has proper handling for undefined Git status:
- When status is undefined, isStatusClean defaults to true via the nullish coalescing operator
- The getPullButtonStatus helper has additional fallback defaults
- Test coverage confirms the behavior in various states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check pull button status handling across the codebase ast-grep --pattern 'getPullBtnStatus({ $$$ isStatusClean: $_, $$$ })'Length of output: 77
Script:
#!/bin/bash # Search for pull button related code and status checks rg -l "pull.*button|pullBtn" --type ts --type tsx # Then look for status clean checks rg "isStatusClean|statusClean" --type ts --type tsx -A 2 # Find component that uses these props ast-grep --pattern 'const $_ = ({$$$isStatusClean$$$}: $_) => { $$$ }'Length of output: 241
Script:
#!/bin/bash # Search for pull button status function rg "getPullBtnStatus" -A 5 # Look for status clean usage rg "isStatusClean" -A 3 # Find pull button related code rg "pull.*button|pullBtn" -A 3Length of output: 121319
brayn003
added
Git Product
## Description - Fixes initial loading state for status - Includes module instances and source module count - Fixes discard flow ## 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/12692930817> > Commit: 6689c8e > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12692930817&attempt=4" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Fri, 10 Jan 2025 08:32:22 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 a new focus on source modules instead of packages - Enhanced tracking of module instances and source module changes - **Bug Fixes** - Updated status change calculations to more accurately reflect module modifications - Improved handling of artifact identification in discard operations - **Refactor** - Renamed and restructured various status-related interfaces and functions - Modified selectors and reducers to support new module tracking approach - **Chores** - Updated type definitions and selector implementations - Removed deprecated package-related tracking mechanisms <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
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/12692930817
Commit: 6689c8e
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 10 Jan 2025 08:32:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores