feat: adding branch persistence #36622
Conversation
WalkthroughThe recent changes introduce enhancements to Git branch management within the application. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (1)
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
|
|
/build-deploy-preview |
github-actions
bot
added
Git Platform
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11118618128. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
295-299: Let's clean up by removing obsolete commented-out code.Keeping old code commented out can clutter your files and make it harder to maintain. Since you've implemented a new approach, you can safely remove the old commented code to enhance readability.
Apply this diff to remove the commented code:
- // const params: any = {}; - // if (showGitBadge) { - // params.branch = showGitBadge; - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/src/pages/Applications/ApplicationCard.tsx (3 hunks)
- app/client/src/sagas/GitSyncSagas.ts (3 hunks)
- app/client/src/sagas/InitSagas.ts (0 hunks)
- app/client/src/utils/storage.ts (2 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/sagas/InitSagas.ts
🔇 Additional comments (7)
app/client/src/utils/storage.ts (2)
46-46: Very good addition of a new storage key, class!The new storage key
LATEST_GIT_BRANCHhas been added to theSTORAGE_KEYSobject. This is a crucial step in maintaining consistency across our local storage operations. Well done!
1116-1132: 🛠️ Refactor suggestionWell done on implementing the getLatestGitBranchFromLocal function!
This function complements the
setLatestGitBranchInLocalfunction we just reviewed. Let's analyze its implementation:
- It takes a single parameter:
baseApplicationId.- It retrieves the stored branches using the
LATEST_GIT_BRANCHkey.- It returns the branch associated with the given
baseApplicationId, orundefinedif not found.- Proper error handling is implemented with logging.
The implementation is correct and follows good practices. However, there's a small improvement we can make:
Instead of returning
nullin the catch block, consider returningundefined. This will make the function's return type more consistent. Here's the suggested change:} catch (error) { log.error("An error occurred while fetching LATEST_GIT_BRANCH"); log.error(error); - return null; + return undefined; }This change will ensure that the function always returns either a string (the branch name) or
undefined, making it easier for the calling code to handle the return value.Let's make sure these new functions are used correctly throughout the codebase:
This will help us ensure that these new functions are being used appropriately in the codebase.
✅ Verification successful
Excellent observation!
The suggestion to returnundefinedinstead ofnullin thegetLatestGitBranchFromLocalfunction enhances type consistency. Additionally, the shell script confirms that bothsetLatestGitBranchInLocalandgetLatestGitBranchFromLocalare correctly utilized across the codebase, ensuring that this change won't introduce any issues.
- Next Steps:
- Proceed with the refactor to return
undefined.- Consider adding TypeScript type annotations to enforce consistent return types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new Git branch storage functions echo "Checking usage of setLatestGitBranchInLocal:" rg "setLatestGitBranchInLocal" --type ts echo "\nChecking usage of getLatestGitBranchFromLocal:" rg "getLatestGitBranchFromLocal" --type tsLength of output: 888
app/client/src/pages/Applications/ApplicationCard.tsx (2)
56-56: Good job importing the necessary utility function.By importing
getLatestGitBranchFromLocal, you're enabling the component to access the latest Git branch from local storage. Well done!
120-122: Excellent initialization of state variables.Setting up
baseApplicationId,showGitBadge, and initializingparamsproperly prepares your component's state for the new Git branch functionality. Keep up the good work!app/client/src/sagas/GitSyncSagas.ts (3)
138-138: Correctly ImportingsetLatestGitBranchInLocalThe import statement adds
setLatestGitBranchInLocalfrom"utils/storage", which is appropriate for enhancing branch persistence.
381-381: RetrievingbaseApplicationIdfor Branch PersistenceIntroducing
baseApplicationIdby selecting it from the state ensures that the application ID is available for subsequent operations related to branch persistence.
409-409: Persisting the Latest Git Branch LocallyBy invoking
setLatestGitBranchInLocal(baseApplicationId, trimmedBranch), the code correctly stores the latest Git branch in local storage, which aligns with the objective of enhancing branch persistence.
|
Deploy-Preview-URL: https://ce-36622.dp.appsmith.com |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/client/src/utils/storage.ts (1)
1122-1139: Excellent work on this function! It's almost perfect, but let's add a small improvement.The
getLatestGitBranchFromLocalfunction is well-implemented, with proper error handling and efficient use of optional chaining. However, we can make it even better!Here's a small suggestion to improve code readability:
- const branch = userBranches?.[baseApplicationId] ?? null; + const branch = userBranches?.[baseApplicationId] || null;Using the logical OR operator (
||) instead of the nullish coalescing operator (??) in this case is more appropriate, as it will returnnullfor bothundefinedand empty string values, which are both falsy in JavaScript.Keep up the excellent work, and remember, small improvements can make a big difference!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/entities/Engine/AppEditorEngine.ts (3 hunks)
- app/client/src/pages/Applications/ApplicationCard.tsx (5 hunks)
- app/client/src/utils/storage.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Applications/ApplicationCard.tsx
🧰 Additional context used
📓 Learnings (1)
app/client/src/utils/storage.ts (1)
Learnt from: brayn003 PR: appsmithorg/appsmith#36622 File: app/client/src/utils/storage.ts:1091-1114 Timestamp: 2024-10-01T07:58:44.643Z Learning: In the `setLatestGitBranchInLocal` function, the nullish coalescing operator (`??`) is already correctly used to handle `null` and `undefined` values in `storedBranches`.
🔇 Additional comments (2)
app/client/src/utils/storage.ts (2)
1092-1120: Well done, class! This function is a shining example of good coding practices.The
setLatestGitBranchInLocalfunction is implemented correctly and efficiently. It properly handles nested objects, preserves existing data, and includes error logging. Keep up the good work!
1091-1139: Class, let's review what we've learned from these excellent additions!These new functions,
setLatestGitBranchInLocalandgetLatestGitBranchFromLocal, are valuable additions to our storage utility. They allow us to efficiently manage Git branch information for different users and applications. Here's why they deserve a gold star:
- They follow consistent error handling and logging practices.
- They use modern JavaScript features like optional chaining and nullish coalescing.
- They're designed to work with nested data structures, preserving existing data.
These functions will greatly improve our ability to manage Git branch information in local storage. Well done on implementing these features!
…ranch-persistence
This comment was marked as duplicate.
This comment was marked as duplicate.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11141797574. |
|
Deploy-Preview-URL: https://ce-36622.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/entities/Engine/AppEditorEngine.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/client/src/entities/Engine/AppEditorEngine.ts (4)
53-53: Excellent inclusion of necessary importsWell done importing
getAIPromptTriggeredandsetLatestGitBranchInLocalfrom"utils/storage". This ensures you have access to the functions needed for tracking AI prompts and setting the latest Git branch locally.
71-73: Correct imports for user selection and loggingGood job importing
getCurrentUserand theUsertype. Includinglogfrom"loglevel"is appropriate for handling logging within your application. These imports are essential for managing user data and logging errors effectively.
275-277: Proper retrieval of current user and branch informationYou're correctly using
yield selectto retrievecurrentUserandcurrentBranchfrom the Redux store. This is a good practice to access the current state within your saga functions.
278-284: Appropriate null checks before setting the latest Git branchIt's commendable that you've added checks for
currentUser?.email,currentApplication?.baseId, andcurrentBranchbefore invokingsetLatestGitBranchInLocal. This helps prevent potential runtime errors by ensuring all necessary data is available.
…ranch-persistence
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts (1)
18-40: A well-structured test case, but let's make it even better!Your test case is logically structured and covers the main functionality of branch persistence. You've done a great job using feature flag interception, generating unique identifiers, and making thorough assertions.
However, I have a small suggestion to improve your work:
Consider replacing
cy.get("@guid")withcy.get<string>("@guid")to explicitly type the alias. This will make your code more type-safe and easier to understand. Here's how you can do it:cy.get<string>("@guid").then((uid) => { // ... rest of your code });Keep up the excellent work, and remember, attention to detail is key in software testing!
app/client/cypress/support/Pages/GitSync.ts (2)
457-460: Well done on implementing the AssertBranchName method!Class, let's take a moment to appreciate this well-structured method. It correctly asserts both the visibility of the branch button and its content. However, to make it even better, we could consider adding a more specific assertion message.
Consider modifying the
AssertContainscall to include a custom error message:- this.agHelper.AssertContains(branch); + this.agHelper.AssertContains(branch, `Branch name should be "${branch}"`);This will make it clearer what exactly is being asserted if the test fails.
462-467: Excellent work on the AssertBranchNameInUrl method!Class, this method is a great example of how to check URL parameters in Cypress. It's concise and focused on its task. However, to maintain consistency with our
agHelperusage throughout the class, we could refactor this slightly.Consider refactoring the method to use
agHelperfor consistency:public AssertBranchNameInUrl(branch: string) { - cy.location("search") - .then((searchParams) => new URLSearchParams(searchParams)) - .invoke("get", "branch") - .should("equal", branch); + this.agHelper.GetCurrentURL().then((url) => { + const searchParams = new URLSearchParams(new URL(url).search); + const urlBranch = searchParams.get("branch"); + this.agHelper.AssertEqual(urlBranch, branch, `URL branch parameter should be "${branch}"`); + }); }This refactoring maintains the same functionality while aligning with the class's use of
agHelperfor assertions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts (1 hunks)
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitConnectV2_spec.ts (0 hunks)
- app/client/cypress/support/Pages/GitSync.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/GitConnectV2_spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/GitSync.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts (2)
1-10: Well done on your imports and variable declarations, class!Your imports are neatly organized, and you've declared your variables clearly at the top of the file. This is good practice for maintaining readable and organized code. Keep up the good work!
12-17: Excellent use of tags and description, students!Your test suite is well-organized with a clear description and appropriate tags. This will make it easier for your classmates to understand the purpose of the test and categorize it correctly. Good job!
app/client/cypress/e2e/Regression/ClientSide/Git/GitPersistBranch_spec.ts
Outdated
Show resolved
Hide resolved
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
brayn003
added
ok-to-test
|
@brayn003 Cypress changes LGTM Let's run this multiple times on EE as well |
|
/ci-test-limit-count run_count=5 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11324183245. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11324183245. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
Description
Persisting latest branch. Re-opening the branch opens it in the latest branch
Fixes #30321
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/11325191494
Commit: 570c6fb
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 14 Oct 2024 10:27:07 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation