chore: added import override CE #40462
Conversation
WalkthroughThis update introduces a comprehensive Git artifact import override feature across both client and server codebases. On the client side, new Redux state, selectors, actions, and React components handle the display and logic for an import override modal when a duplicate artifact is detected. The import modal and saga workflows are updated to support override flows, with new error codes and messaging. On the server, new error types, DTO fields, and error handling logic are added to distinguish duplicate artifact import errors. Several service interfaces and implementations are refactored for modularity, improved error handling, and support for the new override behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ImportModal/OverrideModal
participant Redux
participant Saga
participant Server
User->>UI: Initiate Git Import
UI->>Redux: Dispatch import action
Redux->>Saga: Saga observes import action
Saga->>Server: Send import request (override: false)
Server-->>Saga: Error (Duplicate key)
Saga->>Redux: Set import override details
Redux->>UI: Show ImportOverrideModal
User->>UI: Confirm override
UI->>Redux: Dispatch import with override: true
Redux->>Saga: Saga observes import action
Saga->>Server: Send import request (override: true)
Server-->>Saga: Success
Saga->>Redux: Reset import override details
Redux->>UI: Close modal and update UI
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/pages/Applications/index.tsx(2 hunks)app/client/src/git/components/ImportOverrideModal/index.tsx(1 hunks)app/client/src/git/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/client/src/ce/pages/Applications/index.tsx (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#38171
File: app/client/src/git/ce/components/GitModals/index.tsx:1-7
Timestamp: 2024-12-16T19:45:00.807Z
Learning: The imports in `GitModals` for `ConnectModal`, `DisableAutocommitModal`, `DisconnectModal`, and `SettingsModal` are correct and should not be flagged.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
🔇 Additional comments (4)
app/client/src/git/index.ts (1)
9-9: Export looks good.The new export statement follows the existing pattern for Git-related exports.
app/client/src/ce/pages/Applications/index.tsx (2)
138-138: Import looks good.The import statement is correctly added to the existing Git imports.
149-149:❓ Verification inconclusive
Missing props for GitImportOverrideModal.
The component is rendered without any props. While
artifactTypehas a default value, consider whether a specific artifact type should be provided for better user experience.
🏁 Script executed:
#!/bin/bash # Check usage patterns of similar Git modals in the codebase rg -A 2 -B 2 "GitImportModal|GitRepoLimitErrorModal" --type tsxLength of output: 93
🏁 Script executed:
#!/bin/bash # Locate the file defining GitImportOverrideModal file=$(rg -l "GitImportOverrideModal" -g "*.tsx" | head -n1) echo "Found component in: $file" # Show the component declaration/signature rg -nE "function GitImportOverrideModal|const GitImportOverrideModal" "$file" -A 5 # Show the props interface for the modal rg -n "interface.*Props" "$file" -A 5 # Show where artifactType default is set (if any) rg -n "artifactType" "$file" -A 2Length of output: 1081
#!/bin/bash # 1. Show import line for GitImportOverrideModal in index.tsx rg -n "import .*GitImportOverrideModal" -n "from" app/client/src/ce/pages/Applications/index.tsx # 2. Search across the repo for the component definition rg -nE "export (function|const) GitImportOverrideModal" -g "*.tsx" # 3. If not found, search by filename rg -l "GitImportOverrideModal" -g "*.tsx" -g "*.ts"
Please confirm GitImportOverrideModal’s required props
I don’t see any props passed to
<GitImportOverrideModal />. WhileartifactTypemay have a default, it’s worth verifying whether we should supply a specific value (or any other props) here for clarity and improved UX.
- Check the
GitImportOverrideModalsignature/props interface to confirm which props are required or recommended.- If
artifactTypeshould differ from the default, pass it explicitly:{/* before */}<GitImportOverrideModal />
<GitImportOverrideModal artifactType="YourDesiredType" /* …other props… */ />app/client/src/git/components/ImportOverrideModal/index.tsx (1)
18-20: Interface looks good, but may need expansion.The interface is properly defined with an optional artifact type, but consider if additional props are needed for full functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (12)
app/client/src/git/ce/constants/messages.tsx(1 hunks)app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx(1 hunks)app/client/src/git/components/ImportOverrideModal/index.tsx(1 hunks)app/client/src/git/hooks/useImport.ts(2 hunks)app/client/src/git/hooks/useMessage.ts(1 hunks)app/client/src/git/requests/gitImportRequest.ts(1 hunks)app/client/src/git/requests/gitImportRequest.types.ts(1 hunks)app/client/src/git/store/actions/uiActions.ts(3 hunks)app/client/src/git/store/gitGlobalSlice.ts(2 hunks)app/client/src/git/store/helpers/initialState.ts(1 hunks)app/client/src/git/store/selectors/gitGlobalSelectors.ts(1 hunks)app/client/src/git/store/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/git/ce/constants/messages.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/git/components/ImportOverrideModal/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
app/client/src/git/store/types.ts (1)
app/client/src/git/requests/gitImportRequest.types.ts (1)
GitImportRequestParams(5-13)
app/client/src/git/store/actions/uiActions.ts (2)
app/client/src/git/store/types.ts (1)
GitGlobalReduxState(95-104)app/client/src/git/requests/gitImportRequest.types.ts (1)
GitImportRequestParams(5-13)
app/client/src/git/hooks/useImport.ts (3)
app/client/src/git/store/selectors/gitGlobalSelectors.ts (2)
selectImportOverrideModalOpen(17-18)selectImportOverrideParams(20-21)app/client/src/git/requests/gitImportRequest.types.ts (1)
GitImportRequestParams(5-13)app/client/src/git/store/gitGlobalSlice.ts (1)
gitGlobalActions(55-55)
app/client/src/git/store/gitGlobalSlice.ts (1)
app/client/src/git/store/actions/uiActions.ts (2)
resetImportOverrideParamsAction(51-55)setImportOverrideParamsAction(57-64)
app/client/src/git/store/selectors/gitGlobalSelectors.ts (1)
app/client/src/git/store/types.ts (1)
GitRootState(116-118)
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (2)
app/client/src/git/hooks/useMessage.ts (1)
useMessage(3-12)app/client/src/git/ce/constants/messages.tsx (1)
IMPORT_OVERRIDE_MODAL(7-13)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (18)
app/client/src/git/hooks/useMessage.ts (1)
3-12: LGTM - A clean utility hook for message interpolation.The hook implementation is efficient and properly memoized. It correctly replaces placeholders using regex with their corresponding values from the args object.
Consider adding null/undefined checks for the msg parameter to make the hook more robust.
export default function useMessage(msg: string, args: Record<string, string>) { return useMemo(() => { + if (!msg) return ""; const msgWithArgs = msg.replace(/\{\{([^}]+)\}\}/g, (match, p1) => { // p1 is the key from {{key}} in the message return args[p1] || match; }); return msgWithArgs; }, [msg, args]); }app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (2)
50-68: Modal implementation looks good.The modal structure is well-organized with appropriate components for header, body, and footer. The callbacks are properly memoized.
11-11:❌ Incorrect review comment
Import path inconsistency detected.
The import statement references "git/ee/constants/messages" but based on the AI summary, messages are defined in "git/ce/constants/messages.tsx".
🏁 Script executed:
#!/bin/bash # Check if both paths exist and their content echo "Checking for CE constants path..." [ -f "app/client/src/git/ce/constants/messages.tsx" ] && echo "CE path exists" || echo "CE path doesn't exist" echo "Checking for EE constants path..." [ -f "app/client/src/git/ee/constants/messages.tsx" ] && echo "EE path exists" || echo "EE path doesn't exist" echo "Content comparison if both exist:" if [ -f "app/client/src/git/ce/constants/messages.tsx" ] && [ -f "app/client/src/git/ee/constants/messages.tsx" ]; then echo "Diffing files:" diff "app/client/src/git/ce/constants/messages.tsx" "app/client/src/git/ee/constants/messages.tsx" | head -n 20 fiLength of output: 1372
Import path for EE constants is correct.
Both
app/client/src/git/ce/constants/messages.tsxapp/client/src/git/ee/constants/messages.tsxdefine
IMPORT_OVERRIDE_MODAL. Importing from"git/ee/constants/messages"is intentional for the Enterprise Edition build. No changes required.Likely an incorrect or invalid review comment.
app/client/src/git/store/helpers/initialState.ts (1)
171-171: Proper initialization of the new Redux state property.Adding
importOverrideParams: nullfollows the established pattern for the Git module's state management.app/client/src/git/requests/gitImportRequest.types.ts (1)
12-12: Clean addition of override flag.The optional boolean property maintains backward compatibility while enabling the new override functionality.
app/client/src/git/store/types.ts (2)
23-23: Added import for GitImportRequestParams type.The import is correctly added to support the new state property.
102-102: New state property added for import override parameters.The
importOverrideParamsproperty is properly typed asGitImportRequestParams | nullto store parameters related to Git import override requests. This aligns with the PR objective of adding import override functionality.app/client/src/git/requests/gitImportRequest.ts (1)
20-23: Added override parameter handling to import request.The implementation correctly:
- Extracts the
overrideparameter with a default value offalse- Reconstructs the request body to include this parameter
- Maintains backward compatibility
This change enables the backend to know when an override is intended during import operations.
app/client/src/git/store/gitGlobalSlice.ts (2)
13-17: Updated imports to include new action creators.The import statement is properly refactored to include the new actions for managing import override parameters.
49-50: Added reducers for managing import override parameters.Two new reducers are correctly added:
resetImportOverrideParams: To clear the override parameters statesetImportOverrideParams: To set the override parametersThis follows the established Redux pattern in the codebase and completes the state management for the import override feature.
app/client/src/git/store/selectors/gitGlobalSelectors.ts (1)
17-22: Added selectors for import override modal state.Two well-implemented selectors:
selectImportOverrideModalOpen: Correctly uses double negation to derive modal open state from the existence of parametersselectImportOverrideParams: Properly retrieves parameters with null fallbackThese selectors follow the established pattern and provide a clean interface for components to access the import override state.
app/client/src/git/hooks/useImport.ts (4)
6-7: Import statements correctly addedThese imports are necessary for the new import override functionality.
33-36: Selectors implemented properlyGood addition of selectors for the import override modal state and parameters.
37-46: Callbacks implemented with best practicesBoth callbacks are properly implemented with useCallback for performance optimization and have the correct dependency array.
54-57: Return values properly structuredThe new properties are well-named and consistent with the existing pattern in the hook.
app/client/src/git/store/actions/uiActions.ts (3)
6-6: Import statement correctly addedThe GitImportRequestParams type import is necessary for the new action creators.
51-55: Reset action implemented correctlyThis action follows the established pattern for reset actions and properly sets the state to null.
57-64: Set action implemented correctlyThe action creates a shallow copy of the payload, which is appropriate for this use case since the parameters are stored as-is.
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (2)
34-35: Simplify null handling with default empty stringsThe component sets default values to null but then converts them to empty strings in the useMessage call. This adds unnecessary complexity.
- newArtifactName = null, - oldArtifactName = null, + newArtifactName = "", + oldArtifactName = "",And then:
const modalDescription = useMessage(IMPORT_OVERRIDE_MODAL.DESCRIPTION, { - newArtifactName: newArtifactName ?? "", - oldArtifactName: oldArtifactName ?? "", + newArtifactName, + oldArtifactName, artifactType, });Also applies to: 41-43
72-79: Consider disabling the button during loading stateWhile the component prevents multiple submissions by checking isImportLoading, also disabling the button would provide better visual feedback to users.
<Button isLoading={isImportLoading} + disabled={isImportLoading} onClick={handleImport} size="md" type="submit" > {ctaBtnText} </Button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/client/src/git/ce/constants/messages.tsx(1 hunks)app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx(1 hunks)app/client/src/git/components/ImportOverrideModal/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/git/ce/constants/messages.tsx
- app/client/src/git/components/ImportOverrideModal/index.tsx
🧰 Additional context used
🧠 Learnings (1)
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (1)
Learnt from: brayn003
PR: appsmithorg/appsmith#40462
File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40
Timestamp: 2025-04-28T16:16:02.133Z
Learning: The Appsmith team prefers not to include HTML markup in string constants. Text styling or emphasis should be handled through proper React components rather than HTML tags in strings.
🧬 Code Graph Analysis (1)
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (2)
app/client/src/git/hooks/useMessage.ts (1)
useMessage(3-12)app/client/src/git/ce/constants/messages.tsx (1)
IMPORT_OVERRIDE_MODAL(7-13)
⏰ 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-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx (2)
64-66: Use consistent text styling approachPer the learnings from previous reviews, the team prefers to avoid HTML markup in string constants. The current implementation correctly uses the Text component for plain text rendering.
59-82: Modal implementation looks goodThe modal structure and implementation follow good practices using the component library. The callbacks are properly memoized and the component handles loading states appropriately.
app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/git/store/actions/gitImportActions.ts (2)
52-57: Consider adding a docstring for maintainability.While the function is self-explanatory, adding a brief comment would help developers understand the intended use case for this reset action.
+/** + * Action to reset the Git import state, clearing any loading state and errors. + * Used when cleaning up after import operations or when closing import-related UI. + */ export const resetGitImportAction = (state: GitGlobalReduxState) => { state.gitImport.loading = false; state.gitImport.error = null; return state; };
52-57: Consider consistency with other action creators.Other action creators in this file use a
PayloadActionparameter (even when unused) for consistency. Consider following the same pattern for this new action.-export const resetGitImportAction = (state: GitGlobalReduxState) => { +export const resetGitImportAction = ( + state: GitGlobalReduxState, + // need this here to preserve interface + // eslint-disable-next-line @typescript-eslint/no-unused-vars + action: PayloadAction<void>, +) => { state.gitImport.loading = false; state.gitImport.error = null; return state; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
app/client/src/git/ce/constants/messages.tsx(1 hunks)app/client/src/git/components/ImportModal/index.tsx(3 hunks)app/client/src/git/components/ImportOverrideModal/index.tsx(1 hunks)app/client/src/git/hooks/useImport.ts(3 hunks)app/client/src/git/store/actions/gitImportActions.ts(1 hunks)app/client/src/git/store/gitGlobalSlice.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/client/src/git/ce/constants/messages.tsx
- app/client/src/git/hooks/useImport.ts
- app/client/src/git/components/ImportModal/index.tsx
- app/client/src/git/store/gitGlobalSlice.ts
- app/client/src/git/components/ImportOverrideModal/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/client/src/git/store/actions/gitImportActions.ts (1)
app/client/src/git/store/types.ts (1)
GitGlobalReduxState(95-104)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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 (1)
app/client/src/git/store/actions/gitImportActions.ts (1)
52-57: Clean implementation of action for resetting import state.The added
resetGitImportActionfunction properly resets the Git import state by setting both loading to false and clearing any errors.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
371-373:hydrateLatestis now a no-op – is this still needed?The new implementation just wraps the incoming
ArtifactinMono.just, providing no hydration logic.
If the previous behaviour purposely re-queried or enriched the artifact, this change may introduce stale data bugs; if the method is indeed obsolete, consider removing it and inlining the call sites to keep the class lean.
278-281: Potential memory hit when loading all datasources into a list
getAllByWorkspaceIdWithStorages(..).collectList()materialises the entire datasource set in memory.
If a workspace can host many datasources, consider streaming withbuffer/windowor pushing the conflict check down to the DB/service layer instead of collecting the whole list.app/client/src/instrumentation/index.ts (1)
140-149: PreferObject.entriesto avoid inherited keys
for…inwalks the prototype chain; if someone passes an object with polluted prototypes you’d stringify unintended keys. UseObject.entries/Object.keysto stay on safe ground.- if (hint) { - for (const key in hint) { - if (typeof hint[key] === "string") { - context[key] = hint[key]; - } else { - context[key] = JSON.stringify(hint[key]); - } - } - } + if (hint) { + for (const [key, value] of Object.entries(hint)) { + context[key] = + typeof value === "string" ? value : JSON.stringify(value); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (66)
app/client/package.json(1 hunks)app/client/src/AppErrorBoundry.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx(2 hunks)app/client/src/actions/pageActions.tsx(2 hunks)app/client/src/api/helpers/validateJsonResponseMeta.ts(2 hunks)app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts(1 hunks)app/client/src/api/interceptors/response/failureHandlers/handleNotFoundError.ts(2 hunks)app/client/src/api/interceptors/response/failureHandlers/handleUnauthorizedError.ts(2 hunks)app/client/src/ce/sagas/PageSagas.tsx(2 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(2 hunks)app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx(2 hunks)app/client/src/components/editorComponents/ErrorBoundry.tsx(2 hunks)app/client/src/components/propertyControls/TabControl.tsx(2 hunks)app/client/src/ee/sagas/index.tsx(2 hunks)app/client/src/git/sagas/helpers/handleApiErrors.ts(2 hunks)app/client/src/index.tsx(1 hunks)app/client/src/instrumentation/PageLoadInstrumentation.ts(0 hunks)app/client/src/instrumentation/generateTraces.ts(1 hunks)app/client/src/instrumentation/index.ts(4 hunks)app/client/src/instrumentation/sendFaroErrors.ts(0 hunks)app/client/src/pages/Editor/Canvas.tsx(2 hunks)app/client/src/pages/UserAuth/Login.tsx(2 hunks)app/client/src/pages/UserAuth/SignUp.tsx(2 hunks)app/client/src/pages/UserAuth/VerifyUser.tsx(2 hunks)app/client/src/pages/UserAuth/helpers.ts(2 hunks)app/client/src/reducers/evaluationReducers/treeReducer.ts(2 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts(2 hunks)app/client/src/sagas/AppThemingSaga.tsx(2 hunks)app/client/src/sagas/ErrorSagas.tsx(2 hunks)app/client/src/sagas/EvalErrorHandler.ts(5 hunks)app/client/src/sagas/EvaluationsSaga.ts(3 hunks)app/client/src/sagas/FormEvaluationSaga.ts(2 hunks)app/client/src/sagas/InitSagas.ts(4 hunks)app/client/src/sagas/ReplaySaga.ts(4 hunks)app/client/src/sagas/WidgetLoadingSaga.ts(2 hunks)app/client/src/sagas/layoutConversionSagas.ts(2 hunks)app/client/src/utils/Analytics/sentry.ts(0 hunks)app/client/src/utils/getPathAndValueFromActionDiffObject.ts(2 hunks)app/client/src/utils/helpers.test.ts(2 hunks)app/client/src/utils/helpers.tsx(2 hunks)app/client/src/widgets/CurrencyInputWidget/widget/index.tsx(4 hunks)app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx(2 hunks)app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts(2 hunks)app/client/src/widgets/MapChartWidget/component/utilities.ts(2 hunks)app/client/src/widgets/PhoneInputWidget/widget/index.tsx(2 hunks)app/client/src/widgets/RichTextEditorWidget/component/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx(2 hunks)app/client/src/widgets/TabsMigrator/widget/index.tsx(2 hunks)app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx(4 hunks)app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx(2 hunks)app/client/src/workers/Evaluation/errorModifier.ts(2 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(3 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ErrorType.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/GitConnectDTO.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(8 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java(2 hunks)
💤 Files with no reviewable changes (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java
- app/client/src/instrumentation/sendFaroErrors.ts
- app/client/src/utils/Analytics/sentry.ts
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCECompatibleImpl.java
- app/client/src/instrumentation/PageLoadInstrumentation.ts
✅ Files skipped from review due to trivial changes (27)
- app/client/src/utils/getPathAndValueFromActionDiffObject.ts
- app/client/package.json
- app/client/src/widgets/JSONFormWidget/fields/useRegisterFieldValidity.ts
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/ErrorType.java
- app/client/src/pages/UserAuth/VerifyUser.tsx
- app/client/src/api/interceptors/response/failureHandlers/handleUnauthorizedError.ts
- app/client/src/widgets/MapChartWidget/component/utilities.ts
- app/client/src/sagas/layoutConversionSagas.ts
- app/client/src/git/sagas/helpers/handleApiErrors.ts
- app/client/src/actions/pageActions.tsx
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/GitConnectDTO.java
- app/client/src/pages/UserAuth/helpers.ts
- app/client/src/api/helpers/validateJsonResponseMeta.ts
- app/client/src/sagas/FormEvaluationSaga.ts
- app/client/src/components/editorComponents/ErrorBoundry.tsx
- app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/FormRender.tsx
- app/client/src/pages/UserAuth/SignUp.tsx
- app/client/src/AppErrorBoundry.tsx
- app/client/src/widgets/CurrencyInputWidget/widget/index.tsx
- app/client/src/ce/utils/AnalyticsUtil.tsx
- app/client/src/ce/sagas/PageSagas.tsx
- app/client/src/components/editorComponents/CodeEditor/EvaluatedValuePopup.tsx
- app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithErrorCode.java
- app/client/src/sagas/EvaluationsSaga.ts
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts
- app/client/src/api/interceptors/response/failureHandlers/handleNotFoundError.ts
- app/client/src/instrumentation/generateTraces.ts
🧰 Additional context used
🧠 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (2)
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Learnt from: sondermanish
PR: appsmithorg/appsmith#36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-10-08T15:32:34.114Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
🧬 Code Graph Analysis (22)
app/client/src/ee/sagas/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts (2)
app/client/src/api/types.ts (1)
ApiResponse(14-18)app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/reducers/evaluationReducers/treeReducer.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/workers/Evaluation/errorModifier.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/pages/UserAuth/Login.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/AppThemingSaga.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/pages/Editor/Canvas.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/components/propertyControls/TabControl.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/WidgetLoadingSaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/ErrorSagas.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/utils/helpers.test.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/widgets/TabsMigrator/widget/index.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/InitSagas.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/ReplaySaga.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/utils/helpers.tsx (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
app/client/src/sagas/EvalErrorHandler.ts (1)
app/client/src/instrumentation/index.ts (1)
appsmithTelemetry(184-184)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-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: server-spotless / spotless-check
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (61)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1)
80-80: New method signature added for package.json reconstruction.This method signature
Mono<Object> reconstructPackageJsonFromGitRepository(Path repoSuffix)provides a contract for Git repository package.json reconstruction functionality. The implementation is correctly declared to return a reactiveMonoto maintain consistency with other methods in this interface.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (2)
40-40: Added import for DuplicateKeyException.This import is necessary for the specific error handling added later in the file.
535-538: Improved error handling for duplicate artifacts during import.Added specific error handling for duplicate key exceptions during artifact import. This properly separates duplicate key errors from generic import errors, allowing more precise error reporting to the client.
Note: This implementation correctly follows the established pattern of not including the original Throwable cause in the AppsmithException, as mentioned in the project learnings.
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java (1)
879-886: New error type for duplicate key conflicts during artifact import.This new error enum properly defines a server-side error for duplicate key conflicts during artifact import. The HTTP status code, error messages, and error type classification all follow the established pattern.
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (2)
87-87: Changed field visibility from private to protected.The visibility of
gitServiceConfigandschedulerfields has been changed from private to protected to allow access from subclasses. This refactoring supports extensibility of the class.Also applies to: 101-101
1253-1257: Implementation of the package.json reconstruction method.This method properly implements the interface contract by immediately returning an error Mono indicating that the feature is not supported in the CE implementation. This is an appropriate approach for functionality that's only available in an enterprise edition.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
98-98: Access modifier change improves extensibilityChanging
fileUtilsfrom private to protected aligns with the other dependency fields in this class and enables proper subclassing, which is consistent with the CE/EE pattern used throughout the codebase.
830-833: Improved error loggingEnhanced error logging to include the full error object rather than just the message. This provides more context for debugging repository move issues, including the complete stack trace.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)
44-45: New method to retrieve artifact type and identifierThis new method extends the API to return both artifact type and identifier in a single operation, reducing the need for multiple repository queries.
59-60: Overloaded removeRepository with artifact type contextThe addition of the
isArtifactTypeUnknownparameter allows for more flexible repository removal, particularly during error handling in the import flow.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (3)
69-70: Access modifier change improves extensibilityMaking
commonGitFileUtilsandgitRedisUtilsprotected instead of private enables proper subclassing of this implementation, following the CE/EE pattern.
177-180: Implementation of artifact type and identifier retrievalThe implementation zips the artifact type with an empty string as identifier. Note that the current implementation doesn't actually provide a meaningful identifier.
Consider whether a meaningful identifier should be implemented here, or if this is intentionally setting up for future enhancement.
272-281: Handles unknown artifact types gracefullyThis implementation gracefully handles the case when the artifact type is unknown by defaulting to
APPLICATIONtype. This makes the import flow more robust when dealing with repositories where the artifact type can't be determined initially.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
246-251: Repository clean-up insideonErrorResumelooks solid – good catch!
UsingusingWhento ensureremoveRepository(..)runs on failures prevents temp repos from leaking on disk. Nicely handled.app/client/src/components/propertyControls/TabControl.tsx (1)
13-13: Update to Error Capturing LogicThe code has been updated to use the
appsmithTelemetrysingleton instance for error capturing instead of the directcaptureExceptionimport. This aligns with the centralized telemetry approach used across the codebase.Also applies to: 134-140
app/client/src/pages/Editor/Canvas.tsx (1)
22-22: Error Handling UpdateError reporting has been updated to use the centralized
appsmithTelemetrysingleton. The error object and context data remain unchanged, maintaining consistent error diagnostics.Also applies to: 123-123
app/client/src/ee/sagas/index.tsx (1)
5-5: Standardized Error ReportingThe saga error handler now uses the standardized
appsmithTelemetryservice for exception reporting. This ensures consistent error reporting across the application.Also applies to: 25-27
app/client/src/api/interceptors/response/failureHandlers/handleMissingResponseMeta.ts (1)
3-3: API Error Handling RefactorThe missing response metadata interceptor now correctly uses the application's telemetry service. This change maintains the same error context data while aligning with the centralized approach to error reporting.
Also applies to: 9-15
app/client/src/index.tsx (1)
35-41: LGTM: Improved instrumentation loading pattern.The change to unconditionally load the instrumentation module with proper error handling is a good approach. This ensures telemetry is consistently initialized while gracefully handling any loading failures.
app/client/src/workers/Evaluation/errorModifier.ts (2)
16-16: Standardized telemetry imports.Correctly updated to use the centralized telemetry singleton pattern.
227-229: Updated error capture method.Updated to use the singleton's capture method while maintaining the same error reporting parameters.
app/client/src/reducers/evaluationReducers/treeReducer.ts (2)
6-6: Standardized telemetry imports.Correctly updated to use the centralized telemetry singleton.
35-41: Updated error capture method.Updated to use the singleton's capture method while maintaining the same error context details.
app/client/src/widgets/TabsMigrator/widget/index.tsx (3)
15-15: Standardized telemetry imports.Correctly updated to use the centralized telemetry singleton.
23-28: Updated error capture method in migrateTabsDataUsingMigrator.Successfully migrated to the new telemetry API while preserving all error context.
120-124: Updated error capture method in migrateTabsData.Successfully migrated to the new telemetry API while preserving all error context.
app/client/src/widgets/JSONFormWidget/fields/CurrencyInputField.tsx (2)
17-17: Import updated to use centralized telemetry service.The import has been updated to use the centralized
appsmithTelemetrysingleton from theinstrumentationmodule, which is part of the broader refactoring effort.
135-137: Error tracking updated to use the new telemetry system.This change updates the error capturing methodology to use the centralized telemetry system, which will improve error tracking consistency across the application.
app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
23-23: Import updated to use centralized telemetry service.The import has been updated to use the centralized
appsmithTelemetrysingleton from theinstrumentationmodule, which aligns with the application-wide telemetry refactoring.
230-232: Error tracking updated to use the new telemetry system.The error capturing method has been updated to use the standardized
appsmithTelemetry.captureExceptionmethod, which maintains the same error context while utilizing the new centralized telemetry service.app/client/src/widgets/RichTextEditorWidget/component/index.tsx (1)
463-469:✅ Verification successful
Markdown pattern trigger configuration updated for TinyMCE compatibility.
The
trigger: "space"property has been removed from the text pattern configurations. This change aligns with the TinyMCE version downgrade from 7.7.1 to 6.8.3, allowing the editor to use default pattern triggering behavior instead of explicitly requiring a space key.
🏁 Script executed:
#!/bin/bash # Check if the TinyMCE version has been downgraded in package.json grep -A 2 "@tinymce/tinymce-react" app/client/package.jsonLength of output: 171
Markdown pattern trigger configuration updated for TinyMCE v6 compatibility
Removed the explicit
trigger: "space"from all text-pattern rules. The@tinymce/tinymce-reactdependency has been downgraded to^6.0.0inapp/client/package.jsonso that TinyMCE v6’s default space-triggered behavior applies.app/client/src/widgets/PhoneInputWidget/widget/index.tsx (2)
41-41: Import updated to use centralized telemetry service.The import has been updated to use the centralized
appsmithTelemetrysingleton from theinstrumentationmodule, which is consistent with the application-wide telemetry refactoring pattern.
351-353: Error tracking updated to use the new telemetry system.The error capturing implementation has been updated to use the standardized
appsmithTelemetry.captureExceptionmethod, providing consistent error reporting while maintaining the same error context information.app/client/src/utils/helpers.tsx (2)
47-47: Update to centralized telemetry import looks good.The import change from standalone
captureExceptiontoappsmithTelemetryaligns with the ongoing centralization of telemetry handling.
947-953: Usage of centralized telemetry is correct.The implementation correctly uses the new
appsmithTelemetry.captureExceptionmethod with the appropriate error object and options parameter.app/client/src/widgets/wds/WDSPhoneInputWidget/widget/index.tsx (2)
23-23: Import looks good.Correctly importing the centralized telemetry instance.
166-168: Error capturing implementation is correct.Properly captures exceptions with the widget-specific error name.
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (4)
32-32: Import is correct.Good usage of the centralized telemetry module.
192-194: Error handling implementation looks good.Properly captures exceptions in the
onValueChangemethod.
253-255: Error handling is consistent.Good implementation of error capturing in the
onFocusChangemethod.
318-320: Error handling is complete.Correctly handles errors in the
formatTextmethod, maintaining consistent error name.app/client/src/pages/UserAuth/Login.tsx (2)
55-55: Telemetry import is correct.Good transition to the centralized telemetry service.
119-121: Login error handling is properly implemented.Correctly captures login errors with appropriate error name.
app/client/src/sagas/ReplaySaga.ts (4)
11-11: LGTM: Updated import for telemetry.The import was properly updated to use the centralized telemetry instance.
135-139: LGTM: Updated error reporting method.Error reporting now uses the centralized telemetry instance, maintaining the same error metadata.
170-170: LGTM: Updated error reporting method.Error reporting now uses the centralized telemetry instance, maintaining the same error metadata.
265-265: LGTM: Updated error reporting method.Error reporting now uses the centralized telemetry instance, maintaining the same error metadata.
app/client/src/utils/helpers.test.ts (2)
19-34: LGTM: Updated mock implementation for telemetry.The test correctly mocks the new telemetry implementation, providing both the
getTraceAndContextandcaptureExceptionmethods.
561-563: LGTM: Updated mock implementation usage.The test now correctly uses the mocked telemetry instance for the implementation of
captureException.app/client/src/sagas/WidgetLoadingSaga.ts (2)
19-19: LGTM: Updated import for telemetry.The import was properly updated to use the centralized telemetry instance.
104-106: LGTM: Updated error reporting method.Error reporting now uses the centralized telemetry instance, maintaining the same error metadata.
app/client/src/widgets/TableWidgetV2/component/cellComponents/InlineCellEditor.tsx (2)
23-23: LGTM: Updated import for telemetry.The import was properly updated to use the centralized telemetry instance.
240-242: LGTM: Updated error reporting method.Error reporting now uses the centralized telemetry instance, maintaining the same error metadata.
app/client/src/sagas/ErrorSagas.tsx (1)
44-44: Good refactoring to use centralized telemetry singletonThe change from direct
captureExceptionimport to using theappsmithTelemetrysingleton follows good practices for centralizing error reporting. This maintains the same error metadata while standardizing the telemetry approach.Also applies to: 301-307
app/client/src/sagas/InitSagas.ts (3)
95-95: Correctly migrated to centralized telemetry singletonThe error handling for consolidated API failures has been properly updated to use the
appsmithTelemetrysingleton, maintaining the same error context and metadata.Also applies to: 281-287
377-377: Appropriate error handling updateThe AppEngine error capture is correctly migrated to use the centralized telemetry service.
474-476: Error handling properly standardizedSession validation error reporting is correctly updated to use the centralized telemetry approach while preserving the error context.
app/client/src/sagas/AppThemingSaga.tsx (1)
47-47: Well-executed telemetry refactoringThe error reporting for theme fetching failures has been properly migrated to use the centralized telemetry singleton while preserving all the detailed error context needed for diagnostics.
Also applies to: 129-141
app/client/src/sagas/EvalErrorHandler.ts (2)
20-20: Consistent telemetry approach for cyclical dependency errorsError reporting for cyclical dependencies has been standardized to use the centralized telemetry service while preserving the detailed context including dependency maps and diffs.
Also applies to: 238-251
271-274: Comprehensive telemetry standardization across all error typesAll error types in the evaluation error handling switch statement have been systematically updated to use the centralized telemetry approach. This maintains consistent error reporting while preserving specific error contexts for each error type.
Also applies to: 278-281, 284-287, 292-298, 308-312, 315-319, 323-326, 334-334, 339-342
app/client/src/instrumentation/index.ts (1)
163-181: Double-check Faro measurement schema
pushMeasurementsetstype: "measurement"but Faro’s typings already infer the envelope; duplicating the field may be redundant or rejected depending on SDK version. Please verify against the current SDK contract (it was previously{ values, context }).
...r/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
Show resolved
Hide resolved
…appsmith into chore/git-import-override-1
| if (jsonStr) { | ||
| try { | ||
| artifactNames = JSON.parse(jsonStr); | ||
| } catch {} |
There was a problem hiding this comment.
Ideally we should log the error here
Description
EE PR https://github.com/appsmithorg/appsmith-ee/pull/7251
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/14727350382
Commit: 79ceda0
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 29 Apr 2025 09:54:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Improvements