chore: git pkg - package integration with git #39342
Conversation
brayn003
changed the title
github-actions
bot
added
Git Product
WalkthroughThis pull request introduces a new saga function to handle Git application imports, updating the control flow for Git import operations. It adds a new generator, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/src/git/store/actions/gitImportActions.ts (1)
29-38: Consider adding return type annotationThe action creator would benefit from an explicit return type annotation for better type safety.
export const gitImportSuccessAction = ( state: GitGlobalReduxState, // need this here to preserve interface // eslint-disable-next-line @typescript-eslint/no-unused-vars action: GitImportSuccessPayload, -) => { +): GitGlobalReduxState => { state.gitImport.loading = false; return state; };app/client/src/git-artifact-helpers/application/sagas/applicationImportFromGitSaga.ts (3)
17-20: Consider adding error handling for missing application data.The early return when application is null could benefit from error logging or user notification.
- if (!application) return; + if (!application) { + toast.show("Failed to import application: Invalid response data", { + kind: "error", + }); + return; + }
25-30: Type assertion can be replaced with type guard.The type assertion for
applicationis redundant since we've already validated its existence.- application: application as ApplicationResponsePayload, + application,
34-40: Consider extracting page finding logic to a utility function.The default page finding logic could be reused elsewhere in the application.
+const getDefaultPageId = (pages: Array<{isDefault: boolean, baseId: string}>) => { + const defaultPage = pages.find((page) => !!page.isDefault); + return defaultPage ? defaultPage.baseId : ""; +}; - if (application.pages && application.pages.length > 0) { - const defaultPage = application.pages.find( - (eachPage) => !!eachPage.isDefault, - ); - - basePageId = defaultPage ? defaultPage.baseId : ""; - } + basePageId = application.pages?.length + ? getDefaultPageId(application.pages) + : "";app/client/src/git/sagas/gitImportSaga.ts (1)
40-51: Consider standardizing error modal handling.The repo limit error case has special handling. Consider standardizing error modal behavior for all error types.
if (error) { yield put(gitGlobalActions.gitImportError({ error })); - if (GitErrorCodes.REPO_LIMIT_REACHED === error.code) { + const errorModalConfig = { + [GitErrorCodes.REPO_LIMIT_REACHED]: { + modalAction: gitGlobalActions.toggleRepoLimitErrorModal, + }, + // Add other error types here + }; + + if (errorModalConfig[error.code]) { yield put(gitGlobalActions.toggleImportModal({ open: false })); - yield put(gitGlobalActions.toggleRepoLimitErrorModal({ open: true })); + yield put(errorModalConfig[error.code].modalAction({ open: true })); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/git-artifact-helpers/application/sagas/applicationImportFromGitSaga.ts(1 hunks)app/client/src/git-artifact-helpers/application/sagas/index.ts(1 hunks)app/client/src/git/requests/gitImportRequest.types.ts(2 hunks)app/client/src/git/sagas/gitImportSaga.ts(1 hunks)app/client/src/git/store/actions/gitImportActions.ts(2 hunks)app/client/src/git/store/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (5)
app/client/src/git/requests/gitImportRequest.types.ts (1)
14-19:❓ Verification inconclusive
Verify handling of optional fields in consumers
The
applicationandpackagefields are now optional inGitImportResponseData. Ensure all consumers handle these nullable fields appropriately.
🏁 Script executed:
#!/bin/bash # Search for direct usage of these fields rg "\.application\." --type ts rg "\.package\." --type tsLength of output: 887
Important: Validate Optional Field Handling in Consumers
- Consumers using the
applicationfield (e.g., inapp/client/src/usagePulse/index.ts,ApplicationSagas.tsx, and related tests) are already performing null-checks.- No direct consumers for the
packagefield were detected. Please manually verify that this optional field is safe for future use and—if needed—update consumer code or add tests/documentation to address potential null issues.app/client/src/git/store/index.ts (1)
20-20: LGTM!The export follows the established pattern and is necessary for the new Git import functionality.
app/client/src/git-artifact-helpers/application/sagas/index.ts (1)
15-16: LGTM!The saga integration follows the established pattern using
takeLatestfor handling Git import success.app/client/src/git/store/actions/gitImportActions.ts (1)
26-28: LGTM!The new type definition enhances type safety for Git import success actions.
app/client/src/git/sagas/gitImportSaga.ts (1)
35-38: LGTM! Clean implementation of success path.The success path has been simplified to focus on essential actions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/git/store/actions/gitImportActions.ts (1)
29-34: Consider documenting the interface preservation requirement.The comment "need this here to preserve interface" could use more context. Consider adding a brief explanation of why the action parameter is required but unused.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/git/store/actions/gitImportActions.ts(2 hunks)
⏰ 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-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/src/git/store/actions/gitImportActions.ts (3)
2-10: LGTM! Well-organized type imports.The separation of imports improves readability and the imported types align well with the Git import functionality.
26-27: LGTM! Clean type composition.The
GitImportSuccessPayloadtype effectively combines the async success payload with the import response data.
1-51:❓ Verification inconclusive
Verify integration with applicationImportFromGitSaga.
The changes look good, but let's verify the integration with the new saga mentioned in the AI summary.
🏁 Script executed:
#!/bin/bash # Search for usage of GitImportSuccessPayload in the saga ast-grep --pattern 'type GitArtifactPayloadAction<GitImportSuccessPayload>'Length of output: 73132
Manual Verification Required: Saga Integration Check
The updated Git import actions look good overall. However, our automated search using
ast-grepdid not conclusively reveal any direct usage ofGitImportSuccessPayloadin the expected saga (applicationImportFromGitSaga). Please manually verify that the saga properly integrates and handles the payloads defined in this file to ensure complete end-to-end coverage.
Description
Fixes #38505
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/13409662643
Commit: 820025a
Cypress dashboard.
Tags:
@tag.GitSpec:
Wed, 19 Feb 2025 10:39:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Chores
.lensto the.gitignorefile to exclude it from version control.