-
Notifications
You must be signed in to change notification settings - Fork 4.4k
chore: now branch updates after consolidated api load #41492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/entities/Engine/AppViewerEngine.ts (1)
50-50: Remove or justify the unusedapplicationIdparameter.The
applicationIdparameter is declared but never used in the method body. If it's not needed, remove it; if it's for future use or interface consistency, prefix it with an underscore (_applicationId) to indicate it's intentionally unused.🔎 Proposed fix if parameter is not needed
- *loadGit(applicationId: string, rootSpan: Span) { + *loadGit(rootSpan: Span) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/entities/Engine/AppViewerEngine.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 37830
File: app/client/packages/git/src/actions/checkoutBranchActions.ts:11-18
Timestamp: 2024-11-29T05:38:54.262Z
Learning: In the Git Redux actions (`app/client/packages/git/src/actions`), the `createSingleArtifactAction` function already handles loading state and error management, so additional checks in action creators are unnecessary.
📚 Learning: 2024-10-11T09:28:32.636Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 36622
File: app/client/src/entities/Engine/AppEditorEngine.ts:0-0
Timestamp: 2024-10-11T09:28:32.636Z
Learning: In the file `app/client/src/entities/Engine/AppEditorEngine.ts`, the IDs (`currentApplication.baseId`) and branches (`currentBranch`) are obfuscated, so it's acceptable to log them for debugging purposes.
Applied to files:
app/client/src/entities/Engine/AppViewerEngine.ts
📚 Learning: 2024-12-15T17:45:48.303Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38171
File: app/client/src/git/components/DefaultBranch/DefaultBranchCE.tsx:1-14
Timestamp: 2024-12-15T17:45:48.303Z
Learning: In `app/client/src/git/components/DefaultBranch/DefaultBranchCE.tsx`, the feature flag check is performed at a higher level, so it's acceptable to have `isGitProtectedFeatureLicensed={false}` in this component.
Applied to files:
app/client/src/entities/Engine/AppViewerEngine.ts
📚 Learning: 2024-12-11T08:25:39.197Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38088
File: app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts:40-43
Timestamp: 2024-12-11T08:25:39.197Z
Learning: In `app/client/src/git/components/GitContextProvider/hooks/useGitBranches.ts`, the `useMemo` hook includes dependencies `artifactType` and `baseArtifactId` in its dependency array.
Applied to files:
app/client/src/entities/Engine/AppViewerEngine.ts
🧬 Code graph analysis (1)
app/client/src/entities/Engine/AppViewerEngine.ts (3)
app/client/src/instrumentation/types.ts (1)
Span(15-15)app/client/src/instrumentation/generateTraces.ts (2)
startNestedSpan(42-63)endSpan(65-67)app/client/src/git/helpers/updateBranchParam.ts (1)
updateBranchParam(4-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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 (3)
app/client/src/entities/Engine/AppViewerEngine.ts (3)
36-37: LGTM: Imports are appropriate.Both imports are correctly used in the
loadGitmethod below.
51-51: LGTM: Tracing span management is correct.The span creation and cleanup follows the established pattern used in other methods of this class.
Also applies to: 66-66
55-64: The state pathstate.ui.applications.currentApplication?.gitApplicationMetadata?.branchNameis correct and properly populated from the consolidated API response. No changes needed—the current implementation follows established codebase patterns where synchronous side effects likehistory.replace()are called directly without wrapping incall()effects (57 direct calls vs. 0 wrapped in sagas).Likely an incorrect or invalid review comment.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/20603821286
Commit: 8bec483
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 30 Dec 2025 19:41:56 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.