chore: git mod - integration app #38315
Conversation
![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 (6)
app/client/src/selectors/gitModSelectors.ts (4)
1-1: Track temporary file removalConsider creating a tracking issue for removing this file once Git modularization is fully rolled out to prevent technical debt accumulation.
Would you like me to create a GitHub issue to track the removal of this temporary file?
20-31: Consider adding return type annotationWhile the implementation is correct, adding an explicit return type would improve type safety and documentation.
export function selectGitCurrentBranch( state: AppState, artifactDef: GitArtifactDef, -) { +): string {
33-44: Add return type for consistencySimilar to the previous function, adding an explicit return type would improve type safety.
export function selectGitProtectedMode( state: AppState, artifactDef: GitArtifactDef, -) { +): boolean {
46-57: Optimize selector compositionThe current implementation might cause unnecessary recalculations due to the nested selector. Consider flattening the selectors for better memoization.
export const selectCombinedPreviewMode = createSelector( protectedModeSelector, - (state: AppState) => { - const baseApplicationId = getCurrentBaseApplicationId(state); - - return selectGitProtectedMode( - state, - applicationArtifact(baseApplicationId), - ); - }, + getCurrentBaseApplicationId, + (state: AppState, baseApplicationId: string) => + selectGitProtectedMode(state, applicationArtifact(baseApplicationId)), (isPreviewMode, isProtectedMode) => isPreviewMode || isProtectedMode, );app/client/src/git/components/GitContextProvider/index.tsx (1)
51-57: Consider explicit null check for type safetyThe implementation looks good, but we could make the type checking more explicit.
Consider this minor improvement:
const artifactDef = useMemo(() => { - if (artifactType && baseArtifactId) { + if (artifactType !== null && baseArtifactId !== null) { return { artifactType, baseArtifactId }; } return null; }, [artifactType, baseArtifactId]);This makes the null checking more explicit and TypeScript-friendly.
app/client/src/git/store/selectors/gitArtifactSelectors.ts (1)
114-121: Add TODO explaining interface preservation strategy.While the implementation is correct, consider adding a TODO comment explaining the long-term plan for the interface, as keeping unused parameters might confuse future maintainers.
export const selectCurrentBranch = ( state: GitRootState, - // need this to preserve interface - // eslint-disable-next-line @typescript-eslint/no-unused-vars + // TODO: artifactDef parameter preserved for interface compatibility. + // Plan to remove in future when all consumers are migrated to new state structure. + // eslint-disable-next-line @typescript-eslint/no-unused-vars artifactDef: GitArtifactDef, ) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts(4 hunks)app/client/src/git/components/GitContextProvider/index.tsx(3 hunks)app/client/src/git/components/StatusChanges/StatusChangesView.tsx(2 hunks)app/client/src/git/components/StatusChanges/StatusLoader.tsx(1 hunks)app/client/src/git/components/StatusChanges/StatusTree.tsx(2 hunks)app/client/src/git/components/StatusChanges/types.ts(1 hunks)app/client/src/git/requests/fetchStatusRequest.types.ts(1 hunks)app/client/src/git/store/selectors/gitArtifactSelectors.ts(5 hunks)app/client/src/selectors/gitModSelectors.ts(1 hunks)
🔇 Additional comments (29)
app/client/src/git/requests/fetchStatusRequest.types.ts (1)
27-27: Ensure usage and test coverage for the new field.
You’ve added a numeric modifiedPackages property. Verify that all relevant components handle its potential range (including zero or large values) properly, and confirm that the new field is adequately tested to avoid runtime errors or inconsistencies.
Run the following script to locate references in the codebase and confirm consistent usage:
✅ Verification successful
The modifiedPackages field is properly handled and tested
The field is well-integrated across the codebase with proper null checks and type safety:
- Used in selectors (
gitSyncSelectors.tsx) for status calculations - Properly handled in UI components with null coalescing (
StaticChange.tsx) - Covered in tests (
index.test.tsx) - Correctly transformed in status processing (
applicationStatusTransformer.ts) with proper pluralization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: search for references of the new property 'modifiedPackages' across the repo
rg -A 3 "modifiedPackages"
Length of output: 3941
app/client/src/selectors/gitModSelectors.ts (1)
15-18: LGTM! Clean feature flag implementation
The selector is well-implemented with proper memoization and fallback handling.
app/client/src/git/components/StatusChanges/types.ts (1)
1-5: Solid approach in defining the StatusTreeStruct interface.
The interface is straightforward, typed, and flexible enough to handle hierarchical structures for displaying status messages.
app/client/src/git/components/StatusChanges/StatusLoader.tsx (3)
9-10: Good alignment for consistent spacing.
Switching to a bottom margin ensures a consistent look and feel.
12-13: Better use of styled-components for LoaderText.
Using a styled component is cleaner and reduces inline styling.
20-20: Proper usage of the newly introduced LoaderText.
This makes the loader message more semantically clear and consistent.
app/client/src/git/components/StatusChanges/StatusChangesView.tsx (3)
4-4: Neat reorganization of imports.
Pointing StatusTreeStruct to a separate file and using relative imports for StatusLoader enhances maintainability.
Also applies to: 10-11
14-16: Consistent styling approach with CalloutContainer.
Centralizing the top margin promotes consistency across different components.
57-61: Providing contextual feedback via Callout.
Displaying migrationMessage in a callout is an effective way to inform users about migration-related details.
app/client/src/git/components/StatusChanges/StatusTree.tsx (5)
10-11: Well-structured imports.
Importing styled and StatusTreeStruct aligns perfectly with the refactored architecture.
13-20: Introducing styled components for Collapsible usage.
This enhances stylistic control and keeps consistency with other styled components.
51-52: Smart approach to determine emphasis based on depth.
The noEmphasis flag is a neat way to style nested items differently.
58-58: Passing through the noEmphasis prop.
Consistent use of the noEmphasis flag in nested tree nodes.
64-65: Refactoring to StyledCollapsible and conditional spacing.
Adopting the styled collapsible and adjusting the spacing for child items is a clean and maintainable approach.
Also applies to: 67-67, 69-69, 75-75
app/client/src/git/components/GitContextProvider/index.tsx (2)
6-6: LGTM: Clean type import
The import is properly typed and follows TypeScript best practices.
10-10: Verify null handling in consuming components
The interface changes now allow null values for artifactDef, artifactType, and baseArtifactId. While this adds flexibility, it requires careful null checking in consuming components.
Let's verify the null handling in components that consume these values:
Also applies to: 28-29
✅ Verification successful
Proper null handling is implemented
The codebase shows consistent and safe handling of the nullable artifactDef:
- Optional chaining (
?.) is used when accessingartifactDefproperties in components likeuseGitPermissionsandConnectModal - Null checks using
if (artifactDef)are present in all hooks before dispatching actions - Default values are provided where needed (e.g.,
artifactDef?.artifactType ?? GitArtifactType.Application) - Redux selectors safely handle null values through optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of artifactDef, artifactType, and baseArtifactId
# to ensure proper null checking
# Look for usage of artifactDef from GitContext
ast-grep --pattern 'const { artifactDef } = useGitContext();'
# Look for optional chaining or null checks
rg 'artifactDef\?' app/client/src/git
Length of output: 1856
Script:
#!/bin/bash
# Let's check for direct usage of artifactDef without optional chaining
rg "artifactDef\." app/client/src/git -A 2
# Also check for null checks using if statements
rg "if.*artifactDef" app/client/src/git -A 2
# Let's also check the hooks that destructure artifactDef
ast-grep --pattern 'const { artifactDef } = useGitContext();
$$$
return'
Length of output: 39723
app/client/src/git/store/selectors/gitArtifactSelectors.ts (3)
18-21: LGTM! Function rename improves clarity.
The removal of the redundant "Git" prefix from the selector name is a good improvement, while maintaining the same reliable implementation.
220-222: LGTM! Null safety properly handled.
The addition of null coalescing operator provides proper fallback to an empty array, preventing potential runtime errors.
44-48: Verify modal state handling across components.
The new selector follows the established pattern correctly. Let's verify its usage:
✅ Verification successful
Modal state implementation is properly integrated and used correctly
The new selector is correctly implemented and integrated:
- Used in
useConnecthook to manage modal state - Properly consumed by
ConnectSuccessModalcomponent - Follows the established pattern in the codebase for modal state management
- State updates handled through proper actions in the Redux store
The implementation maintains type safety and follows the existing patterns for Git-related UI state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using this new modal state
rg -l "selectConnectSuccessModalOpen|connectSuccessModalOpen" --type ts --type tsx
Length of output: 112
Script:
#!/bin/bash
# Search for components using this new modal state with correct file type flags
rg -l "selectConnectSuccessModalOpen|connectSuccessModalOpen" --type typescript --type tsx
# Also search for any UI components that might be handling connect success modal
rg -l "ConnectSuccessModal" --type typescript --type tsx
# Look for any modal state handling patterns in git-related components
rg "modal.*open.*git" --type typescript --type tsx
Length of output: 307
Script:
#!/bin/bash
# Search using just file extensions instead of type flags
rg -l "selectConnectSuccessModalOpen|connectSuccessModalOpen" -g "*.ts" -g "*.tsx"
# Search for connect success modal components
rg -l "ConnectSuccessModal" -g "*.ts" -g "*.tsx"
# Look for git-related modal state patterns
rg "modal.*open.*git" -g "*.ts" -g "*.tsx"
# Also check the artifact definition file to understand the modal state structure
fd -e ts -e tsx "GitArtifactDef" --exec cat {}
Length of output: 874
Script:
#!/bin/bash
# Check the implementation of ConnectSuccessModal and its usage
cat app/client/src/git/components/ConnectSuccessModal/index.tsx
cat app/client/src/git/components/ConnectSuccessModal/ConnectSuccessModalView.tsx
# Check how the modal state is used in the connect hook
cat app/client/src/git/hooks/useConnect.ts
# Check the UI state structure in types
cat app/client/src/git/store/types.ts
Length of output: 13351
app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts (10)
3-7: Use of imported constants from messages
This straightforward import looks fine, and the usage aligns well with the existing message structure.
16-21: Icons for new resource types
Assigning these icons appears consistent and provides clear visual cues for each resource.
28-28: Optional 'extra' field
Allowing an optional message is a neat approach for flexible node descriptions.
32-38: Appending 'extra' in createTreeNode
Simple and clear logic for building the final message string.
63-67: Sorting node definitions by subject
Sorting ensures consistent ordering of children in the tree. Nicely done.
147-150: Sorting the final tree
Sorting the top-level nodes by message maintains consistency in display order.
212-239: statusRemoteCountTransformer
Effectively conveys the behind/ahead status with appended helpful messages.
241-256: statusSettingsTransformer
Clean check for application.json modifications with a suitable fallback.
258-273: statusThemeTransformer
Consistent handling for theme changes. No issues found.
320-320: Integration of new transformer calls
All relevant status transformer functions are now aggregated and returned. This finalizes the comprehensive view of changes.
Also applies to: 324-324, 325-325, 326-326
app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/components/editorComponents/ApiResponseView.test.tsx (1)
20-22: Consider testing both preview mode states.
MockingselectCombinedPreviewModeto returnfalseensures a predictable environment for this test. However, consider adding a test case with it returningtrueto cover different preview mode scenarios and validate component behavior under both conditions.app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx (1)
12-14: Add a scenario to validate theselectCombinedPreviewModereturningtrue.Currently, the function is mocked to always return
false. Consider adding a scenario to ensure coverage when the selector returnstrueas well, which will help in validating any logic that depends on the preview mode being enabled.app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts (1)
63-67: Consider memoizing the sort comparator functionThe sort comparator function is recreated on each call. Consider extracting it to a constant for better performance.
+const sortBySubject = (a: TreeNodeDef, b: TreeNodeDef) => + a.subject.localeCompare(b.subject, undefined, { sensitivity: "base" }); function createTreeNodeGroup(nodeDefs: TreeNodeDef[], subject: string) { return { icon: ICON_LOOKUP[nodeDefs[0].type], message: `${nodeDefs.length} ${subject} ${determineVerbForDefs(nodeDefs)}`, children: nodeDefs - .sort((a, b) => - a.subject.localeCompare(b.subject, undefined, { sensitivity: "base" }), - ) + .sort(sortBySubject) .map(createTreeNode), }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/pages/Applications/CreateNewAppsOption.test.tsx(1 hunks)app/client/src/components/editorComponents/ApiResponseView.test.tsx(1 hunks)app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts(4 hunks)
🔇 Additional comments (5)
app/client/src/git/artifact-helpers/application/applicationStatusTransformer.ts (5)
3-28: LGTM: Interface and constant changes are well-structured
The additions to ICON_LOOKUP and TreeNodeDef interface are consistent with the module's purpose and maintain type safety.
212-239: LGTM: Remote count transformer implementation
The function properly handles both ahead and behind counts with appropriate messages.
241-273: LGTM: Settings and theme transformers
Both transformers correctly handle their respective JSON file modifications.
306-306: Typo in plural logic for module instance
Line 306 references modifiedPackages instead of modifiedModuleInstances.
320-326: LGTM: Main transformer function updates
The new transformers are properly integrated into the main transformer function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts (1)
123-127: Consider caching the Git branch selection.The Git branch selection is performed in a generator function that might be called frequently. Consider memoizing the branch selection if performance becomes a concern.
+// Add to the top of the file with other imports +import { createSelector } from 'reselect'; + +const getGitBranchForApp = createSelector( + [getCurrentBaseApplicationId, selectGitCurrentBranch], + (baseApplicationId, gitBranch) => gitBranch +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts(5 hunks)
🔇 Additional comments (3)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts (3)
20-22: LGTM: Clean import additions for Git module integration.
The new imports properly separate concerns between editor state and Git functionality.
91-100: Verify the impact of null branch parameter.
The function now handles null branch parameter gracefully, but we should ensure this doesn't affect existing focus retention behavior.
✅ Verification successful
Null branch parameter handling is safe and consistent with existing behavior
The implementation is correct and safe because:
- The test cases show that the code already expects and handles both formats (
EDITOR_STATE.{id}andEDITOR_STATE.{id}#{branch}) - The same branching logic is consistently used across the codebase, including in the focus retention logic
- All existing usages pass the branch parameter explicitly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of createEditorFocusInfoKey to verify compatibility
rg -A 2 "createEditorFocusInfoKey\(" --type ts
Length of output: 560
Script:
#!/bin/bash
# Let's check how getCurrentFocusInfo is implemented to understand the focus retention behavior
ast-grep --pattern 'function getCurrentFocusInfo($_) { $$$ }'
# Also check for any other functions that might be using the focus info key
rg "EDITOR_STATE\." --type ts -B 2 -A 2
Length of output: 2588
154-158: Ensure consistent branch handling across state transitions.
The branch handling in getEntitiesForStore matches getEntitiesForSet, maintaining consistency. However, verify that the branch state is properly maintained during navigation.
Also applies to: 201-203
✅ Verification successful
Let me search for branch state handling during navigation to verify the concern.
Let me try a different search pattern to analyze the branch handling during navigation.
Branch state handling is properly maintained during navigation
The codebase shows consistent branch handling across state transitions:
AppIDEFocusStrategy.tscorrectly retrieves and uses branch information in bothgetEntitiesForStoreandgetEntitiesForSetmethods- Branch state is properly maintained through selectors (
selectGitCurrentBranch,getCurrentGitBranch) - Navigation components consistently use branch information for state management and URL handling
- Tests verify branch state preservation during navigation (
AppIDEFocusStrategy.test.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential inconsistencies in branch handling
rg -A 3 "branch[\"']?\s*[=:]" --type ts
Length of output: 44321
Script:
#!/bin/bash
# Search for branch handling in navigation-related files
ast-grep --pattern 'function* $FUNC($$$) {
$$$
branch$$$
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Search for navigation-related files and their branch handling
rg -A 5 "getEntitiesForStore|getEntitiesForSet" --type ts
# Also check for branch-related state transitions
rg -A 5 "selectGitCurrentBranch|getCurrentGitBranch" --type ts
Length of output: 30268
There was a problem hiding this comment.
@brayn003 I understand such integration leads to such large PRs but I noticed there were a lot of refactors as well, like filename changes, function changes and new components which could have been avoided and focused more on the integration part. As an author it can be very tempting to do everything in a single PR but it has negative side-effects and especially for reviewing, it shifts the focus from the right stuff and becomes very tedious and time consuming. Let's aim for smaller PRs
| @@ -30,6 +30,7 @@ const blockAirgappedRoutes = (config: InternalAxiosRequestConfig) => { | |||
|
|
|||
| const addGitBranchHeader = (config: InternalAxiosRequestConfig) => { | |||
| const state = store.getState(); | |||
| // ! git mod - not sure how to replace this, we could directly read state if required | |||
There was a problem hiding this comment.
In your git folder where you have API requests; why not create an HOC for the Api class that injects branch as header. IIRC the config property that .get .post accept has an option to pass additional headers
There was a problem hiding this comment.
I would ideally want to remove this from the interceptor logic and pass it with the api explicitly. But not tampering this right now
| basePageId: string, | ||
| branch: string | null = null, | ||
| ) => | ||
| branch |
There was a problem hiding this comment.
Why are we changing these?
There was a problem hiding this comment.
Branch can either be undefined or null. With the old implementation - when branch is not present it would return either EDITOR_STATE.pageid.undefined or EDITOR_STATE.pageid.null. This creates issues. So, I have modified the logic to ignore branch totally when it is either undefined or null
| [gitConnectSuccess.type]: ( | ||
| state: ApplicationsReduxState, | ||
| action: PayloadAction<GitConnectSuccessPayload>, | ||
| ) => { | ||
| return { | ||
| ...state, | ||
| currentApplication: { | ||
| ...state.currentApplication, | ||
| gitApplicationMetadata: | ||
| action.payload.responseData.gitApplicationMetadata, | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Why do we need to do this in the applicationsReducer?
There was a problem hiding this comment.
we do the same thing with old implementation as well. Didn't want to change it too much. Will revisit later
| @@ -179,7 +198,9 @@ export const AppIDEFocusStrategy: FocusStrategy = { | |||
| appState: EditorState.EDITOR, | |||
| params: prevFocusEntityInfo.params, | |||
| }, | |||
| key: `EDITOR_STATE.${prevFocusEntityInfo.params.basePageId}#${branch}`, | |||
| key: branch | |||
| ? `EDITOR_STATE.${prevFocusEntityInfo.params.basePageId}#${branch}` | |||
There was a problem hiding this comment.
Why are we changing this?
There was a problem hiding this comment.
Branch can either be undefined or null. With the old implementation - when branch is not present it would return either EDITOR_STATE.pageid.undefined or EDITOR_STATE.pageid.null. This creates issues. So, I have modified the logic to ignore branch totally when it is either undefined or null
| } | ||
| } | ||
|
|
||
| export const selectCombinedPreviewMode = createSelector( |
There was a problem hiding this comment.
Suggestion
// Selector to get the artifact definition based on the current base application ID
const selectArtifactDef = createSelector(
getCurrentBaseApplicationId,
(baseApplicationId) => applicationArtifact(baseApplicationId)
);
// Improved selectCombinedPreviewMode selector
export const selectCombinedPreviewMode = createSelector(
previewModeSelector,
selectArtifactDef,
(state: AppState) => state, // Pass the entire state for downstream selectors
(isPreviewMode, artifactDef, state) => {
const isProtectedMode = selectGitProtectedMode(state, artifactDef);
return isPreviewMode || isProtectedMode;
}
);| const baseApplicationId: string = yield select(getCurrentBaseApplicationId); | ||
|
|
||
| const branch: string | undefined = yield select( | ||
| selectGitCurrentBranch, | ||
| applicationArtifact(baseApplicationId), | ||
| ); |
There was a problem hiding this comment.
Can this be a selector like getCurrentBranchForApplication? I noticed this block being used in multiple places
|
Yes my bad 🙈 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/client/src/entities/Engine/AppEditorEngine.ts (1)
27-27: Remove commented import statementThe
historyimport is commented out on line 27 but then imported again on line 71. Remove the commented import to maintain clean code.-// import history from "utils/history";Also applies to: 71-71
app/client/src/sagas/FocusRetentionSaga.ts (1)
126-128: Removing focus history per branch
Including the branch in the focus-history key is a solid step to ensure that focus states remain per-branch.If you need to handle multiple branch transitions, consider caching or cleaning old keys to prevent stale focus states.
app/client/src/selectors/gitModSelectors.ts (1)
1-2: Temporary file notice
A clear comment indicating temporary status. Keep a reminder to remove or finalize the file once the feature is fully rolled out.app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts (1)
121-123: Consider type consistency with createEditorFocusInfoThe branch type should match the
createEditorFocusInfoparameter type for consistency.- const branch: string | undefined = yield select( + const branch: string | null = yield select( selectGitApplicationCurrentBranch, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts(5 hunks)app/client/src/ce/reducers/index.tsx(3 hunks)app/client/src/ce/sagas/PageSagas.tsx(4 hunks)app/client/src/ce/sagas/index.tsx(2 hunks)app/client/src/entities/Engine/AppEditorEngine.ts(5 hunks)app/client/src/entities/Engine/index.ts(2 hunks)app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx(2 hunks)app/client/src/sagas/ActionExecution/StoreActionSaga.ts(2 hunks)app/client/src/sagas/FocusRetentionSaga.ts(2 hunks)app/client/src/sagas/GlobalSearchSagas.ts(1 hunks)app/client/src/selectors/gitModSelectors.ts(1 hunks)app/client/src/widgets/withWidgetProps.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/ce/sagas/index.tsx
- app/client/src/widgets/withWidgetProps.tsx
- app/client/src/ce/reducers/index.tsx
- app/client/src/entities/Engine/index.ts
- app/client/src/ce/sagas/PageSagas.tsx
🔇 Additional comments (20)
app/client/src/pages/Editor/GlobalHotKeys/GlobalHotKeys.tsx (1)
49-49: Nice import usage.
This new import name is clear and consistent with the broader gitModSelectors naming.
app/client/src/entities/Engine/AppEditorEngine.ts (3)
326-333: LGTM: Recent entities restoration implementation
The implementation correctly handles the restoration of recent entities with proper null checks for the application ID and optional branch parameter.
379-389: LGTM: Git module feature flag implementation
Clean implementation of the Git module initialization with proper feature flag check.
390-402: Verify cleanup of forked saga
The forked saga might need cleanup when the component unmounts. Consider using a cancellable pattern.
Also, consider optimizing the disabled state:
} else {
const currentBranch: string = yield select(
selectGitApplicationCurrentBranch,
);
- // init of temporary remote url from old application
- yield put(remoteUrlInputValue({ tempRemoteUrl: "" }));
// add branch query to path and fetch status
if (currentBranch) {
history.replace(addBranchParam(currentBranch));
yield fork(this.loadGitInBackground);
}
}app/client/src/sagas/ActionExecution/StoreActionSaga.ts (2)
15-15: Switch to updated Git selector
The import of selectGitApplicationCurrentBranch reflects the new approach of obtaining the branch details. Good job keeping the code aligned with the modular Git structure.
24-26: Confirm branch usage
Using selectGitApplicationCurrentBranch here ensures we're always fetching the latest branch info. Verify that all references to the old selector are removed to maintain consistency.
app/client/src/sagas/GlobalSearchSagas.ts (2)
26-26: Adoption of new Git branch selector
Replacing getCurrentGitBranch with selectGitApplicationCurrentBranch is consistent with the modular approach. No issues observed.
35-37: Ensure consistent usage across sagas
This saga now relies on selectGitApplicationCurrentBranch. Double-check all other sagas also use the updated selector to avoid mismatches.
app/client/src/sagas/FocusRetentionSaga.ts (1)
24-24: Updated import for branch retrieval
Swapping out the old getCurrentGitBranch import with selectGitApplicationCurrentBranch is aligned with modular Git enhancements.
app/client/src/selectors/gitModSelectors.ts (8)
3-5: Selective imports
Consolidating the needed selectors in one file helps maintain a single source of truth for Git configurations.
6-9: Modular approach
The git imports for new branch and protected mode selectors align well with the modularization plan.
10-14: Editor selectors
Good usage of existing editor selectors and artifact helpers in tandem with the new Git modular approach.
16-19: Feature flag detection
selectGitModEnabled gracefully defaults to false if the flag is undefined, preventing accidental feature exposure.
21-24: Application artifact resolution
This approach to retrieve application artifacts by base ID keeps the logic clean and consistent.
26-34: Branch selection logic
The selector provides a neat fallback to the old or new branch logic, depending on the feature-flag state. This prevents breaking changes for existing flows.
36-44: Protected mode toggling
Mirroring the branch selection logic for protectedMode ensures consistent gating with the Git mod flag.
46-50: Preview mode combination
Combining the standard preview mode with the Git-protected mode in a single selector is clean and avoids branching logic duplication.
app/client/src/ce/navigation/FocusStrategy/AppIDEFocusStrategy.ts (3)
20-20: LGTM: Import change aligns with Git modularization
The switch to selectGitApplicationCurrentBranch from gitModSelectors is consistent with the Git module integration objectives.
89-98: LGTM: Improved branch handling in key generation
The changes address the previously discussed issues with undefined/null branch handling, providing a cleaner key format.
150-152: Ensure consistent branch handling across focus storage
Two suggestions:
- Align branch type with createEditorFocusInfo
- Verify the changes don't break existing focus storage
- const branch: string | undefined = yield select(
+ const branch: string | null = yield select(
selectGitApplicationCurrentBranch,
);Also applies to: 195-197
brayn003
added
ok-to-test
## Description - Git mod integration with applications behind feature flag Fixes appsmithorg#37815 Fixes appsmithorg#37816 Fixes appsmithorg#37817 Fixes appsmithorg#37818 Fixes appsmithorg#37819 Fixes appsmithorg#37820 ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12526603607> > Commit: 19f3ca0 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12526603607&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Sat, 28 Dec 2024 15:57:13 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 ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced various new hooks for Git functionalities, including `useGitModEnabled`, `useGitCurrentBranch`, `useGitProtectedMode`, and `useGitConnected`. - Added a new component `DeployButton` to manage deployment actions. - Integrated `ConnectSuccessModal` to enhance user feedback upon successful Git connection. - Added `GitImportModal` for improved Git import functionality. - Introduced `GlobalProfileView` for managing and displaying user profile information. - Added a new icon, `CloudIconV2`, to the icon provider. - Implemented `fetchGlobalSSHKey` and `gitImport` sagas for better state management. - **Improvements** - Enhanced handling of Git-related states and actions across multiple components and sagas. - Streamlined selector usage for determining preview mode by replacing `combinedPreviewModeSelector` with `selectCombinedPreviewMode`. - Updated the `DeployPreview` component to manage success feedback and handle commit operations more effectively. - Improved the `StatusChangesView` component by adding a callout for migration messages. - Added new transformers for application status handling. - **Bug Fixes** - Updated error handling and loading states across various actions and components to improve reliability. - **Refactor** - Refactored action creators to use `createArtifactAction` instead of `createSingleArtifactAction` for consistency and clarity. - Consolidated Git import and connection logic to improve modularity and maintainability. - Modified the `Header` component to utilize `GitApplicationContextProvider` for managing Git-related state. - Updated various components to utilize the new `selectCombinedPreviewMode` selector for improved state management. - Refactored the `DeployPreview` component to enhance its functionality and styling consistency. - Enhanced the `applicationStatusTransformer` to handle multiple status transformations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #37815
Fixes #37816
Fixes #37817
Fixes #37818
Fixes #37819
Fixes #37820
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12526603607
Commit: 19f3ca0
Cypress dashboard.
Tags:
@tag.AllSpec:
Sat, 28 Dec 2024 15:57:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
useGitModEnabled,useGitCurrentBranch,useGitProtectedMode, anduseGitConnected.DeployButtonto manage deployment actions.ConnectSuccessModalto enhance user feedback upon successful Git connection.GitImportModalfor improved Git import functionality.GlobalProfileViewfor managing and displaying user profile information.CloudIconV2, to the icon provider.fetchGlobalSSHKeyandgitImportsagas for better state management.Improvements
combinedPreviewModeSelectorwithselectCombinedPreviewMode.DeployPreviewcomponent to manage success feedback and handle commit operations more effectively.StatusChangesViewcomponent by adding a callout for migration messages.Bug Fixes
Refactor
createArtifactActioninstead ofcreateSingleArtifactActionfor consistency and clarity.Headercomponent to utilizeGitApplicationContextProviderfor managing Git-related state.selectCombinedPreviewModeselector for improved state management.DeployPreviewcomponent to enhance its functionality and styling consistency.applicationStatusTransformerto handle multiple status transformations.