fix: removed conditional to check for jslib files

[sondermanish]

Description

• Modified file writing logic for custom js lib writing logic

Fixes #32734

Warning

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.Git"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10699103627
Commit: fe25da5
Cypress dashboard.
Tags: @tag.Git
Spec:

---

Wed, 04 Sep 2024 09:58:48 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

• Yes
• No

Summary by CodeRabbit

• Bug Fixes

  • Simplified the logic for saving JavaScript libraries, which may improve performance but could affect when libraries are saved after modifications.

• Chores

  • Removed unnecessary import statement, streamlining the codebase.

[coderabbitai]

Walkthrough

The changes involve modifications to the updateEntitiesInRepo method within the FileUtilsCEImpl.java file. The primary adjustment is the simplification of the conditional logic that determines when JavaScript libraries are saved, now only checking for non-null modifiedResources. Additionally, an import statement for CollectionUtils has been removed, reflecting a change in the method's dependencies.

Changes

Files	Change Summary
.../appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java	The conditional check in updateEntitiesInRepo was simplified to only verify that modifiedResources is non-null. The previous checks related to resource modifications and custom JavaScript libraries were commented out. The import for CollectionUtils was also removed.

Assessment against linked issues

Objective	Addressed	Explanation
Deleting jslib gives incorrect status in modal (#32734)	❌	The changes do not address the issue of incorrect status messages upon deletion.

Poem

In the code where logic flows,
A simplification gently grows.
Libraries once held in tight embrace,
Now find a new, more open space.
With fewer checks, the path is clear,
To save what’s changed, let’s give a cheer! 🎉

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 2a6885a and 3c76115.

Files selected for processing (1)

• app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (2 hunks)

Additional comments not posted (1)

app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (1)

282-285: Simplification of the JavaScript library saving condition may have unintended consequences.

The changes simplify the condition to only check if modifiedResources is not null, removing the additional checks for modifiedResources.isAllModified() or non-empty modifiedResources.getModifiedResourceMap().get(CUSTOM_JS_LIB_LIST). This may alter the behavior of when JavaScript libraries are saved.

It's important to verify that the new behavior aligns with the expected functionality and doesn't introduce any issues. Can you provide more context on the reasoning behind removing the additional checks? Are there any potential scenarios where JavaScript libraries might be saved unintentionally?

To verify the impact of this change, you can run the following script to search for usages of the updateEntitiesInRepo method and analyze how the modified condition affects the saving of JavaScript libraries:

Review the search results to understand the different scenarios in which updateEntitiesInRepo is called and assess whether the simplified condition introduces any unintended behavior.

Verification successful

---

Verify the impact of the simplified condition within FileUtilsCEImpl.java.

The updateEntitiesInRepo method is only used within the FileUtilsCEImpl.java file, and the modifiedResources object is derived from the applicationGitReference parameter. This means the impact of the simplified condition is limited to the internal logic of this file.

To ensure the new condition aligns with the intended functionality, verify how applicationGitReference is constructed and passed to this method. This will help determine if the removal of additional checks is appropriate and doesn't introduce unintended behavior.

• Review the construction and usage of applicationGitReference within the file to ensure it aligns with the expected behavior.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usages of the `updateEntitiesInRepo` method and analyze the impact of the modified condition.

# Test: Search for the method usage. Expect: Occurrences of the method call with different `modifiedResources` arguments.
rg --type java -A 5 $'updateEntitiesInRepo'

Length of output: 1645

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3c76115 and fe25da5.

Files selected for processing (1)

• app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (2 hunks)

Additional comments not posted (2)

app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java (2)

285-289: ****

The comment block provides valuable insights into the implications of the code change. It highlights the potential performance impact of accessing the customJSLib directory for comparison with valid JavaScript libraries on every save operation.

These considerations should be taken into account when evaluating the overall behavior and performance of the modified code. The comment serves as a helpful documentation and reminder for future maintainers.

---

281-290: Ensure the simplified condition aligns with the intended behavior for saving JavaScript libraries.

The code change removes the additional checks for the allModified flag and CUSTOM_JS_LIB_LIST entries when determining if JavaScript libraries should be saved. This simplifies the condition to only check if modifiedResources is non-null.

While this change streamlines the logic, it's important to ensure that it aligns with the desired functionality. The removed checks may have been in place to prevent unnecessary saving of unmodified JavaScript libraries.

Consider the following:

• Verify that saving JavaScript libraries based solely on modifiedResources being non-null meets the requirements.
• Assess the performance impact of accessing the customJSLib directory for comparison with valid JavaScript libraries on every save operation.
• Add unit tests to cover scenarios where JavaScript libraries are saved or skipped based on the modified condition.

To ensure the simplified condition aligns with the intended behavior, you can perform the following verifications:

1. Review the usage of the updateEntitiesInRepo method and assess if the modified condition introduces any unintended consequences.

2. Analyze the performance impact by profiling the application with the modified condition.

3. Verify the presence of unit tests covering the modified behavior.

Based on the results of these verifications, you can determine if the simplified condition aligns with the intended behavior and if any further adjustments or optimizations are necessary.