chore: minimise fetch page calls and optimise refactoring entity flow #41362
+487
−68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds bulk fetch/update APIs for actions and action collections, implements bulk-processing refactor flows that replace per-item saves, introduces cache-aware refactor paths and DTO cache fields, switches some sets to concurrent key sets, and updates repository, service, and test code to support batched operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RefactorSvc as RefactoringServiceCEImpl
participant DTO as RefactorEntityNameDTO
participant CollSvc as ActionCollectionService
participant ActSvc as NewActionService
participant Repo as Repository
Caller->>RefactorSvc: refactorEntityName(dto, contextId)
alt PAGE context with cache
RefactorSvc->>RefactorSvc: pre-fetch PageDTO -> dto.withCachedPageDTO(...)
end
RefactorSvc->>RefactorSvc: continueRefactorFlow(...) (cache-aware)
RefactorSvc->>CollSvc: findAllByIds(ids, permission) (bulk)
CollSvc->>Repo: execute findAllByIds query
Repo-->>CollSvc: Flux<ActionCollection>
RefactorSvc->>ActSvc: findAllByIds(ids, permission) (bulk)
ActSvc->>Repo: execute findAllByIds query
Repo-->>ActSvc: Flux<NewAction>
RefactorSvc->>RefactorSvc: processBulk(items) -> collect updates
RefactorSvc->>Repo: bulkUpdate(collectedUpdates)
Repo-->>RefactorSvc: ack
RefactorSvc-->>Caller: completion
sequenceDiagram
participant Service
participant BulkProc as processBulk*
participant Repo
Service->>Service: fetchAllByIds(...).collectList()
alt empty list
Service-->>Service: return Mono.empty()
else has items
Service->>BulkProc: processBulk(items)
loop per item
BulkProc->>BulkProc: apply refactor rules, collect updates
end
BulkProc->>Repo: bulkUpdate(collectedUpdates)
Repo-->>BulkProc: ack
BulkProc-->>Service: done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/refactors/applications/RefactoringServiceCEImpl.java (1)
205-215: Restore analytics property population when cache is missing
prepareAnalyticsPropertiesnow writes bothAPPLICATION_IDandPAGE_IDonly whenhasCachedApplicationId()is true. For any flow that doesn’t populate the cache (e.g., non-page contexts or legacy callers), the method exits without recording either property, and we lose the page-level telemetry we’ve always emitted. Please move thePAGE_IDassignment outside the conditional and add the previous fallback to fetch and populateAPPLICATION_IDwhen the cache is empty, so analytics payloads stay complete.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/RefactorEntityNameCE_DTO.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/refactors/PageLayoutRefactoringServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/refactors/applications/RefactoringServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomActionCollectionRepositoryCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactoringServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: NilanshBansal
Repo: appsmithorg/appsmith PR: 33641
File: app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CacheableRepositoryHelperCE.java:23-25
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The new methods `fetchDefaultTenant` and `evictCachedTenant` in the `CacheableRepositoryHelperCE` interface are covered by existing and new tests, as confirmed by NilanshBansal.
Applied to files:
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCEImplTest.java
📚 Learning: 2024-10-22T07:06:20.529Z
Learnt from: rishabhrathod01
Repo: appsmithorg/appsmith PR: 37001
File: app/server/appsmith-server/src/main/java/com/appsmith/server/layouts/UpdateLayoutServiceCEImpl.java:228-229
Timestamp: 2024-10-22T07:06:20.529Z
Learning: In `UpdateLayoutServiceCEImpl.java`, within the `updateLayoutDsl` method, variables `widgetNames` and `executableNames` are always initialized before use, so null checks before calling `size()` on them are unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactoringServiceCEImpl.java
📚 Learning: 2025-03-06T12:23:01.075Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 39594
File: app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java:179-268
Timestamp: 2025-03-06T12:23:01.075Z
Learning: When using Reactor's `flatMap` operator with shared mutable state like ArrayList, synchronization is necessary because `flatMap` may process items concurrently, and operations like ArrayList.add() are not atomic across multiple threads.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java
📚 Learning: 2025-01-30T10:47:35.449Z
Learnt from: ApekshaBhosale
Repo: appsmithorg/appsmith PR: 38925
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java:0-0
Timestamp: 2025-01-30T10:47:35.449Z
Learning: In reactive programming with Spring WebFlux, avoid using `block()` as it can lead to thread pool exhaustion. Instead, maintain the reactive chain using operators like `flatMap()` and handle empty cases with `defaultIfEmpty()`. When adding tags or metrics, ensure they are applied within the reactive chain.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ActionCollectionSpanCE.java (1)
ActionCollectionSpanCE(9-22)
⏰ 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: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
43-51: LGTM - Well-documented unvalidated bulk update method.The Javadoc clearly states this method bypasses validation for performance optimization in refactoring and migration scenarios. Ensure callers pre-validate actions before using this method to avoid persisting invalid data.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
433-451: Implementation and usage are correct—approve as-is.The single caller in
NewActionRefactoringServiceCEImplproperly uses this method within its refactoring pipeline at line 160, processing actions throughextractAndSetJsonPathKeysbefore the bulk update. This aligns perfectly with the documented use case for refactoring operations.
...om/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java
Show resolved
Hide resolved
Comment on lines
+112
to
+167
| return Flux.fromIterable(actions) | ||
| .flatMap(newAction -> { | ||
| ActionDTO action = newActionService.generateActionByViewMode(newAction, false); | ||
|
|
||
| if (action.getActionConfiguration() == null) { | ||
| return Mono.just(newAction); | ||
| } | ||
|
|
||
| // If this is a JS function rename, add this collection for rename | ||
| if (StringUtils.hasLength(action.getCollectionId()) && newName.equals(action.getValidName())) { | ||
| updatableCollectionIds.add(action.getCollectionId()); | ||
| } | ||
|
|
||
| newAction.setUnpublishedAction(action); | ||
|
|
||
| return this.refactorNameInAction(action, oldName, newName, evalVersion, oldNamePattern) | ||
| .map(updates -> { | ||
| if (!updates.isEmpty()) { | ||
| updatedBindingPaths.addAll(updates); | ||
| if (StringUtils.hasLength(action.getCollectionId())) { | ||
| updatableCollectionIds.add(action.getCollectionId()); | ||
| } | ||
| actionsToUpdate.add(newAction); | ||
| updatedActionNames.add(action.getName()); | ||
| } | ||
| return newAction; | ||
| }); | ||
| }) | ||
| .collectList() | ||
| .flatMap(processedActions -> { | ||
| if (actionsToUpdate.isEmpty()) { | ||
| log.debug( | ||
| "No actions require updates for refactoring in {} {}", | ||
| contextType.toString().toLowerCase(), | ||
| contextId); | ||
| return Mono.empty(); | ||
| } | ||
|
|
||
| log.debug( | ||
| "Processing bulk operation for {} actions that require refactoring", | ||
| actionsToUpdate.size()); | ||
|
|
||
| // Extract JSON path keys for all actions that need updating | ||
| return Flux.fromIterable(actionsToUpdate) | ||
| .flatMap(newActionService::extractAndSetJsonPathKeys) | ||
| .collectList() | ||
| .flatMap(updatedActions -> { | ||
| // Use general purpose bulk update for all actions at once | ||
| return newActionService.bulkUpdateActions(updatedActions); | ||
| }) | ||
| .doOnSuccess(unused -> log.debug( | ||
| "Bulk refactoring completed for {} {} - updated actions: {}", | ||
| contextType.toString().toLowerCase(), | ||
| contextId, | ||
| updatedActionNames)); | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid concurrent mutations inside the Reactor pipeline.
Flux.flatMap can process inner publishers concurrently, so mutating shared structures (actionsToUpdate, updatedActionNames, updatableCollectionIds, updatedBindingPaths) from inside the lambda introduces data races and corrupted updates.(pandepra.medium.com) Swap to a sequential operator (e.g. concatMap) or otherwise avoid shared mutable state in this stage.
- return Flux.fromIterable(actions)
- .flatMap(newAction -> {
+ return Flux.fromIterable(actions)
+ .concatMap(newAction -> {Based on learnings
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return Flux.fromIterable(actions) | |
| .flatMap(newAction -> { | |
| ActionDTO action = newActionService.generateActionByViewMode(newAction, false); | |
| if (action.getActionConfiguration() == null) { | |
| return Mono.just(newAction); | |
| } | |
| // If this is a JS function rename, add this collection for rename | |
| if (StringUtils.hasLength(action.getCollectionId()) && newName.equals(action.getValidName())) { | |
| updatableCollectionIds.add(action.getCollectionId()); | |
| } | |
| newAction.setUnpublishedAction(action); | |
| return this.refactorNameInAction(action, oldName, newName, evalVersion, oldNamePattern) | |
| .map(updates -> { | |
| if (!updates.isEmpty()) { | |
| updatedBindingPaths.addAll(updates); | |
| if (StringUtils.hasLength(action.getCollectionId())) { | |
| updatableCollectionIds.add(action.getCollectionId()); | |
| } | |
| actionsToUpdate.add(newAction); | |
| updatedActionNames.add(action.getName()); | |
| } | |
| return newAction; | |
| }); | |
| }) | |
| .collectList() | |
| .flatMap(processedActions -> { | |
| if (actionsToUpdate.isEmpty()) { | |
| log.debug( | |
| "No actions require updates for refactoring in {} {}", | |
| contextType.toString().toLowerCase(), | |
| contextId); | |
| return Mono.empty(); | |
| } | |
| log.debug( | |
| "Processing bulk operation for {} actions that require refactoring", | |
| actionsToUpdate.size()); | |
| // Extract JSON path keys for all actions that need updating | |
| return Flux.fromIterable(actionsToUpdate) | |
| .flatMap(newActionService::extractAndSetJsonPathKeys) | |
| .collectList() | |
| .flatMap(updatedActions -> { | |
| // Use general purpose bulk update for all actions at once | |
| return newActionService.bulkUpdateActions(updatedActions); | |
| }) | |
| .doOnSuccess(unused -> log.debug( | |
| "Bulk refactoring completed for {} {} - updated actions: {}", | |
| contextType.toString().toLowerCase(), | |
| contextId, | |
| updatedActionNames)); | |
| }); | |
| return Flux.fromIterable(actions) | |
| .concatMap(newAction -> { | |
| ActionDTO action = newActionService.generateActionByViewMode(newAction, false); | |
| if (action.getActionConfiguration() == null) { | |
| return Mono.just(newAction); | |
| } | |
| // If this is a JS function rename, add this collection for rename | |
| if (StringUtils.hasLength(action.getCollectionId()) && newName.equals(action.getValidName())) { | |
| updatableCollectionIds.add(action.getCollectionId()); | |
| } | |
| newAction.setUnpublishedAction(action); | |
| return this.refactorNameInAction(action, oldName, newName, evalVersion, oldNamePattern) | |
| .map(updates -> { | |
| if (!updates.isEmpty()) { | |
| updatedBindingPaths.addAll(updates); | |
| if (StringUtils.hasLength(action.getCollectionId())) { | |
| updatableCollectionIds.add(action.getCollectionId()); | |
| } | |
| actionsToUpdate.add(newAction); | |
| updatedActionNames.add(action.getName()); | |
| } | |
| return newAction; | |
| }); | |
| }) | |
| .collectList() | |
| .flatMap(processedActions -> { | |
| if (actionsToUpdate.isEmpty()) { | |
| log.debug( | |
| "No actions require updates for refactoring in {} {}", | |
| contextType.toString().toLowerCase(), | |
| contextId); | |
| return Mono.empty(); | |
| } | |
| log.debug( | |
| "Processing bulk operation for {} actions that require refactoring", | |
| actionsToUpdate.size()); | |
| // Extract JSON path keys for all actions that need updating | |
| return Flux.fromIterable(actionsToUpdate) | |
| .flatMap(newActionService::extractAndSetJsonPathKeys) | |
| .collectList() | |
| .flatMap(updatedActions -> { | |
| // Use general purpose bulk update for all actions at once | |
| return newActionService.bulkUpdateActions(updatedActions); | |
| }) | |
| .doOnSuccess(unused -> log.debug( | |
| "Bulk refactoring completed for {} {} - updated actions: {}", | |
| contextType.toString().toLowerCase(), | |
| contextId, | |
| updatedActionNames)); | |
| }); |
sondermanish
previously approved these changes
Nov 5, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java (1)
109-170: Race condition persists: shared Sets mutated inside concurrent flatMap.The past review flagged this issue, but it remains unresolved.
Flux.flatMapcan process inner publishers concurrently, so mutatingupdatableCollectionIds(lines 124, 134) andupdatedBindingPaths(line 132) from inside the lambda introduces data races. WhileactionsToUpdateandupdatedActionNamesare now synchronized, the Sets passed as parameters are not thread-safe for concurrent access.Apply this diff to enforce sequential processing:
- return Flux.fromIterable(actions) - .flatMap(newAction -> { + return Flux.fromIterable(actions) + .concatMap(newAction -> {Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/RefactoringMetaDTO.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-06T12:23:01.075Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 39594
File: app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/importable/ActionCollectionImportableServiceCEImpl.java:179-268
Timestamp: 2025-03-06T12:23:01.075Z
Learning: When using Reactor's `flatMap` operator with shared mutable state like ArrayList, synchronization is necessary because `flatMap` may process items concurrently, and operations like ArrayList.add() are not atomic across multiple threads.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java
📚 Learning: 2025-01-30T10:47:35.449Z
Learnt from: ApekshaBhosale
Repo: appsmithorg/appsmith PR: 38925
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java:0-0
Timestamp: 2025-01-30T10:47:35.449Z
Learning: In reactive programming with Spring WebFlux, avoid using `block()` as it can lead to thread pool exhaustion. Instead, maintain the reactive chain using operators like `flatMap()` and handle empty cases with `defaultIfEmpty()`. When adding tags or metrics, ensure they are applied within the reactive chain.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java
⏰ 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: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/refactors/ActionCollectionRefactoringServiceCEImpl.java (2)
57-73: Clean refactoring to bulk operations.The switch from per-item fetch to bulk processing is well-structured. The early-return when empty and debug logging are good additions.
85-112: Concurrency concern from past review has been addressed.The current implementation correctly uses
collectList()(line 95) to gather all results before the bulk update. ThecollectionsToUpdateparameter (line 96) is the immutable result fromcollectList(), not a shared mutable list being written from insideflatMap. Since eachactionCollectionis processed independently and mutations occur on distinct objects, concurrent processing viaflatMapis safe here.app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/RefactoringMetaDTO.java (2)
7-7: LGTM: Import addition is correct.The
ConcurrentHashMapimport is necessary for the thread-safe set implementations below.
26-31: Concurrent modifications confirmed—no action required onupdatableCollectionIds.The evidence confirms your concern:
RefactoringMetaDTOis instantiated once inRefactoringServiceCEImpland passed to multiple concurrent refactoring services (ActionCollectionRefactoringServiceCEImpl,NewActionRefactoringServiceCEImpl, etc.). These services read and modifyupdatableCollectionIdsandupdatedBindingPathsfrom different reactor threads, justifyingConcurrentHashMap.newKeySet().Regarding
updatedPage: it follows a write-once pattern (set viasetUpdatedContext(), then read-only), so thread-safety is less critical here—provided the write happens before concurrent reads are scheduled. However, if reactor scheduling doesn't guarantee this visibility, consider markingupdatedPageasvolatileor wrapping it in anAtomicReferencefor defensive programming consistency.
Comment on lines
+17
to
+22
| /** | ||
| * Thread-safe set for tracking binding paths that have been updated during refactoring. | ||
| * Uses ConcurrentHashMap.newKeySet() to ensure thread-safety as this collection can be | ||
| * modified concurrently by multiple refactoring services during reactive operations. | ||
| */ | ||
| Set<String> updatedBindingPaths = ConcurrentHashMap.newKeySet(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Verify that concurrent modification actually occurs.
The switch from HashSet to ConcurrentHashMap.newKeySet() adds overhead. While the Javadoc mentions "modified concurrently by multiple refactoring services during reactive operations," reactive flows don't automatically imply concurrent modification unless threads are explicitly switched via .subscribeOn() or .publishOn().
Run this script to verify concurrent access patterns:
🏁 Script executed:
#!/bin/bash
# Search for concurrent access patterns to updatedBindingPaths
# Look for subscribeOn/publishOn calls that might indicate thread switching
rg -n -C3 'updatedBindingPaths' --type java | rg -C3 '(subscribeOn|publishOn|parallel)'
# Check if RefactoringMetaDTO is shared across reactive chains
rg -n -A5 'RefactoringMetaDTO' --type java | rg -B2 -A3 '(flatMap|zip|merge|concat)'
# Look for documentation or comments about concurrent access
rg -n -C2 'RefactoringMetaDTO.*concurrent|concurrent.*RefactoringMetaDTO' --type javaLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find all usages of updatedBindingPaths field
echo "=== Usages of updatedBindingPaths ==="
rg -n 'updatedBindingPaths' --type java -C2
echo ""
echo "=== Where RefactoringMetaDTO is instantiated ==="
rg -n 'new RefactoringMetaDTO|RefactoringMetaDTO()' --type java -C3
echo ""
echo "=== Classes/methods that use RefactoringMetaDTO ==="
rg -n 'RefactoringMetaDTO' --type java -C1 | head -50Length of output: 45648
🏁 Script executed:
#!/bin/bash
# Check for thread switching operators in the reactive chains that modify updatedBindingPaths
echo "=== Checking refactoring methods that modify updatedBindingPaths ==="
echo "WidgetRefactoringServiceCEImpl - refactorReferencesInExistingEntities:"
rg -n -A30 'public Mono<Void> refactorReferencesInExistingEntities' app/server/appsmith-server/src/main/java/com/appsmith/server/widgets/refactors/WidgetRefactoringServiceCEImpl.java | head -50
echo ""
echo "NewActionRefactoringServiceCEImpl - refactorReferencesInExistingEntities:"
rg -n -A30 'public Mono<Void> refactorReferencesInExistingEntities' app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/refactors/NewActionRefactoringServiceCEImpl.java | head -50
echo ""
echo "=== Looking for thread switching in the composition (RefactoringServiceCEImpl) ==="
rg -n -B5 -A15 'widgetEntityRefactoringService.refactorReferencesInExistingEntities' app/server/appsmith-server/src/main/java/com/appsmith/server/refactors/applications/RefactoringServiceCEImpl.javaLength of output: 4832
Revert updatedBindingPaths to HashSet; concurrent modification does not occur.
The refactoring services execute sequentially via .then() chaining without thread-switching operators. widgetsMono.then(actionsAndCollectionsMono) ensures operations run one after another on the same thread, so ConcurrentHashMap.newKeySet() is unnecessary overhead.
Set<String> updatedBindingPaths = new HashSet<>();🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/RefactoringMetaDTO.java
around lines 17 to 22, replace the thread-safe ConcurrentHashMap.newKeySet()
initialization with a plain HashSet because concurrent modification does not
occur; change the declaration to use new HashSet<>() for updatedBindingPaths and
ensure any necessary imports are adjusted/removed accordingly.
Failed server tests
|
1 similar comment
Failed server tests
|
sondermanish
approved these changes
Nov 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/8275
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19130353944
Commit: 01f27c3
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 06 Nov 2025 10:08:51 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
New Features
Tests