chore: updated the run behaviour toast messages#40653
chore: updated the run behaviour toast messages#40653AmanAgarwal041 wants to merge 7 commits intoreleasefrom
Conversation
WalkthroughA new feature flag and enum constant were introduced to control "automatic" run behavior for executables. The logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OnLoadExecutablesUtilCEImpl
participant FeatureFlagService
participant ExecutableOnLoadService
Client->>OnLoadExecutablesUtilCEImpl: updateExecutablesRunBehaviour(...)
OnLoadExecutablesUtilCEImpl->>FeatureFlagService: isFlagEnabled(release_reactive_actions_enabled)
FeatureFlagService-->>OnLoadExecutablesUtilCEImpl: flag state (true/false)
OnLoadExecutablesUtilCEImpl->>ExecutableOnLoadService: fetch all executables
ExecutableOnLoadService-->>OnLoadExecutablesUtilCEImpl: executables list
OnLoadExecutablesUtilCEImpl->>OnLoadExecutablesUtilCEImpl: Determine run behavior changes
OnLoadExecutablesUtilCEImpl->>OnLoadExecutablesUtilCEImpl: Update messages based on flag and changes
OnLoadExecutablesUtilCEImpl-->>Client: Mono<Boolean> (indicating if updates occurred)
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 skip-tests=true |
github-actions
bot
added
Core Query Execution
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15027503673. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java (1)
253-254: Consider additional test casesThe comment suggests adding more tests. Consider adding tests for:
- Mixed changes with feature flag off
- Using ON_PAGE_LOAD with feature flag on (if supported)
- Edge cases like null run behavior
These would further increase test coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/RunBehaviourEnum.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (17)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/enums/FeatureFlagEnum.java (1)
32-32: LGTM! New feature flag added correctly.The addition of
release_reactive_actions_enabledfollows the established pattern in the enum, positioned appropriately before the EE flags section.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/RunBehaviourEnum.java (1)
8-9: LGTM! Enum updated with new value and proper trailing comma.The new
AUTOMATICenum value is well-documented with a clear comment explaining its purpose.app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilImpl.java (2)
7-7: LGTM! Import added for the FeatureFlagService dependency.Proper import added to support the new constructor parameter.
22-30: Constructor updated correctly to inject the FeatureFlagService.The constructor has been properly updated to accept and pass the
FeatureFlagServiceto the superclass.app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (4)
5-5: LGTM! Import added for FeatureFlagEnum.Required import added to support feature flag checking.
22-22: LGTM! Import added for FeatureFlagService.Proper import added for the new dependency.
95-95: LGTM! Dependency on FeatureFlagService added.New field added to support feature flag checks.
405-443: Conditional toast message implementation looks good.The implementation properly uses the feature flag to conditionally show different toast messages:
- When flag is enabled: messages refer to "automatic" execution and "no longer run automatically"
- When flag is disabled: messages refer to "on page load" execution and "no longer be executed on page load"
This aligns with the PR objective to update toast messages related to run behavior based on the feature flag state.
app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java (9)
35-36: LGTM: Proper dependency injection for feature flagGood job mocking the FeatureFlagService which is a critical dependency for the conditional toast message logic.
42-51: Constructor initialization looks goodThe constructor properly initializes all required dependencies including the feature flag service. Observation-related services are correctly set to null as they're not needed for these tests.
53-65: LGTM: Base case test is comprehensiveGood test for the empty executables scenario, verifying the method returns false and completes.
67-96: LGTM: Good test for legacy behaviorThis test properly verifies the behavior when feature flag is off, confirming the "page load" message appears.
98-129: LGTM: Good test for new behaviorThis test correctly verifies the behavior when feature flag is on, confirming the "automatic" message appears.
131-154: LGTM: Good test for turning off executables with legacy behaviorThis test properly verifies the behavior when turning off executables with feature flag off.
156-180: LGTM: Good test for turning off executables with new behaviorThis test correctly verifies the behavior when turning off executables with feature flag on.
182-222: LGTM: Comprehensive mixed state testGood comprehensive test that verifies both enabling and disabling executables when feature flag is enabled.
224-251: LGTM: Good test for no-change scenariosThis test properly verifies that no messages are added when there's no change in run behavior.
...erver/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
Failed server tests
|
Failed server tests
|
| @JsonView(Views.Internal.class) | ||
| Boolean executeOnLoad; | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
@AmanAgarwal041 Can you pull release into this branch? This change is already made on release
| */ | ||
| @Deprecated | ||
| @JsonView({Views.Internal.class, FromRequest.class, Git.class}) | ||
| @JsonView({Views.Internal.class, FromRequest.class}) |
There was a problem hiding this comment.
This change is also made on release
(cherry picked from commit 9583638)
AmanAgarwal041
force-pushed
the
chore/update-toast-run-behaviour
branch
from
da03474 to
e4b5f15
Compare
…update-toast-run-behaviour
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/15066336640. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java (1)
33-65: Consider adding test for edge cases in feature flag transitionsWhile the tests cover both feature flag states individually, consider adding a test for transitions - what happens when the feature flag changes state and executables need to be updated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (11)
app/server/appsmith-server/src/test/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImplTest.java (5)
1-326: Comprehensive test coverage for the feature flag behaviorThis new test class provides solid test coverage for the refactored behavior in
OnLoadExecutablesUtilCEImpl, with tests for both feature flag states (enabled/disabled) and various run behavior scenarios.
89-123: Well-structured test for feature flag enabled caseThis test effectively validates that when the feature flag is enabled, the proper "automatic" message is generated for executables turned on.
125-159: Test confirms correct message when feature flag is disabledGood validation that the traditional "page load" message is shown when the feature flag is disabled, ensuring backward compatibility.
197-252: Thorough test for multiple executables with mixed state changesThis test comprehensively covers the scenario where some executables are turned on while others are turned off, validating that the correct messages are generated for each.
290-324: Test verifies userSetOnLoad=true behaviorGood test to confirm that executables with
userSetOnLoad=trueare not updated, preserving user-defined settings.app/server/appsmith-server/src/main/java/com/appsmith/server/onload/internal/OnLoadExecutablesUtilCEImpl.java (6)
309-440: Well-structured refactoring for feature flag integrationThe method has been effectively refactored to handle both feature flag states while maintaining clear logic flow. The conditional messages and run behavior handling align with the PR objectives.
323-336: Clean implementation of feature flag-based executable filteringThis filtering logic clearly distinguishes between the two run behaviors (
ON_PAGE_LOADandAUTOMATIC) based on the feature flag state.
404-424: Clear message generation based on feature flag stateThe message generation logic correctly implements different user-facing messages based on the feature flag, matching the PR objectives for toast message updates.
342-357: Well-designed state transition handlingThe code cleanly handles the mapping of executables to their target behavior and computes the differences between existing and new states.
364-380: Respects user-defined settingsThe implementation properly respects the
userSetOnLoadflag, ensuring that user-explicitly-set behaviors are preserved.
95-95: New dependency properly declaredThe
FeatureFlagServiceis correctly added as a dependency to support the feature flag checks.
|
Deploy-Preview-URL: https://ce-40653.dp.appsmith.com |
Failed server tests
|
2 similar comments
Failed server tests
|
Failed server tests
|
…update-toast-run-behaviour
Failed server tests
|
|
Closing this Pr as many unnecessary changes were introduced in this which was making current implementation even more complex. I have created a separate PR where I have updated the toast messages along with solving one other issue. All test cases have passed on that PR, will go ahead and merge that one instead. |
3 participants
Description
Changed the logic to update the message of the actions getting executed on page load or automatic based on the flag
release_reactive_actions_enabled. Added the test case for onLoadExecutables file.Fixes #40471
or
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.Table, @tag.JS, @tag.Datasource"
🔍 Cypress test results
Warning
Tests have not run on the HEAD f042d98 yet
Mon, 19 May 2025 08:09:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests