chore: Removed setting of published theme while application import #38806
Conversation
WalkthroughThe pull request involves modifications to theme and application import services in the Appsmith server. The changes primarily focus on refining the handling of themes during import processes, simplifying theme management logic, and ensuring proper initialization of application-related lists. The modifications aim to improve code clarity, prevent potential null pointer exceptions, and streamline the theme import and publishing mechanisms. Changes
Sequence DiagramsequenceDiagram
participant Import Service
participant Theme Service
participant Application
Import Service->>Theme Service: importEntities()
Theme Service->>Application: Update unpublishedThemeId
Theme Service->>Theme Service: Simplify theme import logic
Theme Service-->>Import Service: Return import result
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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
|
github-actions
bot
added
skip-changelog
![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 (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)
375-376: Consider extracting page list initialization to a helper method.The map creation logic could be moved to a private helper method to improve readability and maintainability.
+ private Map<String, List<ApplicationPage>> createApplicationPageListMap(Application application) { + List<ApplicationPage> unPublishedPages = CollectionUtils.isEmpty(application.getPages()) + ? new ArrayList<>() + : application.getPages(); + List<ApplicationPage> publishedPages = CollectionUtils.isEmpty(application.getPublishedPages()) + ? new ArrayList<>() + : application.getPublishedPages(); + return Map.of(PUBLISHED, publishedPages, UNPUBLISHED, unPublishedPages); + } // In the original method: - List<ApplicationPage> unPublishedPages = CollectionUtils.isEmpty(application.getPages()) - ? new ArrayList<>() - : application.getPages(); - List<ApplicationPage> publishedPages = CollectionUtils.isEmpty(application.getPublishedPages()) - ? new ArrayList<>() - : application.getPublishedPages(); - Map<String, List<ApplicationPage>> mapOfApplicationPageList = - Map.of(PUBLISHED, publishedPages, UNPUBLISHED, unPublishedPages); + Map<String, List<ApplicationPage>> mapOfApplicationPageList = createApplicationPageListMap(application);app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (1)
272-274: Consider adding error handling for getDefaultThemeId failure.While the code structure is good, it might be worth adding explicit error handling in case
getDefaultThemeId()fails.return publishedThemeIdMono.flatMap(publishedThemeId -> saveThemeForApplication( publishedThemeId, editModeTheme, application, ApplicationMode.PUBLISHED)); + }) + .onErrorResume(error -> { + log.error("Error while publishing theme: {}", error.getMessage()); + return Mono.error(new AppsmithException(AppsmithError.THEME_PUBLISH_ERROR)); + });app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (1)
Line range hint
80-93: Consider optimizing Application object creation.Instead of creating a new Application object just to update theme IDs, consider using a builder pattern or creating a utility method for dry run operations.
- Application application = new Application(); - application.setUnpublishedThemeId(editModeThemeId); - application.setId(importableArtifact.getId()); + Application application = Application.builder() + .id(importableArtifact.getId()) + .unpublishedThemeId(editModeThemeId) + .build();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java (1)
367-373: LGTM! Defensive null handling for page lists.The initialization of empty ArrayLists when the pages are null is a good practice to prevent NullPointerExceptions.
app/server/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java (3)
264-266: Documentation accurately reflects the implementation.The comment clearly explains the unique aspect of theme publishing, which helps future maintainers understand the design decision.
267-268: Important context captured in comments.The comment effectively documents the specific cases (import and new application) where this null handling is necessary.
269-270: Well-structured null handling with reactive patterns.Good use of
Mono.justOrEmpty()withswitchIfEmpty()to handle the null published theme ID case. This maintains the reactive nature of the code while providing a clean fallback to the default theme.app/server/appsmith-server/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java (1)
74-77: LGTM! Good variable naming improvement.The rename from
editModeThemetoeditModeThemeMonobetter reflects the reactive nature of the variable.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (5)
1019-1019: LGTM! Verifying published theme handlingThe assertion correctly verifies that published mode theme ID is not set during application import.
1141-1141: LGTM! Theme repository verificationThe test properly verifies theme repository interaction after application import.
1294-1294: LGTM! Consistent theme verificationThe assertion maintains consistency in verifying theme handling behavior across test cases.
4739-4739: LGTM! Theme verification in page importThe assertion correctly verifies theme handling during application import with page modifications.
5291-5291: LGTM! Theme verification with faulty DSLThe assertion maintains consistent theme verification even in error scenarios.
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 #38767
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, @tag.ImportExport"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12912937730
Commit: 0e9f7c2
Cypress dashboard.
Tags:
@tag.Git, @tag.ImportExportSpec:
Wed, 22 Jan 2025 18:43:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Theme Management
Bug Fixes
Testing