chore: add new condition for form conditionals#38556
Conversation
WalkthroughThe pull request refactors the form evaluation logic by removing the old implementations from Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Component
participant Q as QueryPaneSaga
participant A as formEvaluationActions
participant F as FormEvaluationSaga
participant W as formEval Worker
U->>Q: Triggers form change event
Q->>A: Calls startFormEvaluations (with context)
A->>F: Dispatches evaluation action with payload
F->>W: Forwards evaluation request with context
W-->>F: Returns updated evaluation state
F-->>Q: Updates application state accordingly
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
the
skip-changelog
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
|
🔴🔴🔴 Cyclic Dependency Check: 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. |
|
🔴🔴🔴 Cyclic Dependency Check: 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. |
github-actions
bot
added
Frontend
|
🔴🔴🔴 Cyclic Dependency Check: 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. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
ayushpahwa
requested review from
a team
and removed request for
a team and
ApekshaBhosale
![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
🔭 Outside diff range comments (1)
app/client/src/workers/Evaluation/formEval.ts (1)
298-302: 🛠️ Refactor suggestionImprove error handling in evaluateDynamicValuesConfig.
The catch block silently assigns "error" without logging or proper error handling. Consider adding error logging or propagating the error.
Apply this fix:
try { evaluatedValue = eval(value); } catch (e) { - evaluatedValue = "error"; + console.error("Error evaluating dynamic value:", e); + evaluatedValue = `Error: ${e.message}`; } finally {🧰 Tools
🪛 Biome (1.9.4)
[error] 299-299: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
🧹 Nitpick comments (3)
app/client/src/actions/formEvaluationActions.ts (1)
8-14: Consider adding proper type definitions.The
editorConfigandsettingConfigparameters are typed asany. While there are TODO comments acknowledging this, it would be better to define proper types for these configurations.app/client/src/workers/Evaluation/formEval.ts (2)
353-353: Simplify boolean expression.The double negation
!!conditionBlockis redundant as the value will be coerced to a boolean in the if condition.Apply this fix:
- if (!!conditionBlock) { + if (conditionBlock) {🧰 Tools
🪛 Biome (1.9.4)
[error] 353-353: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
4-8: Consider adding JSDoc comments for better documentation.The function
evaluateFormConfigElementsand its parameters lack documentation. Adding JSDoc comments would improve code maintainability.Add documentation like this:
/** * Evaluates form config elements based on the provided configuration * @param actionConfiguration - The action configuration object * @param config - The form config evaluation object * @param editorContextType - The type of editor context (PAGE, WORKFLOWS, MODULE) * @param datasourceConfiguration - Optional datasource configuration * @returns Evaluated form config */ function evaluateFormConfigElements(Also applies to: 313-320
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/actions/evaluationActions.ts(0 hunks)app/client/src/actions/formEvaluationActions.ts(1 hunks)app/client/src/sagas/FormEvaluationSaga.ts(2 hunks)app/client/src/sagas/QueryPaneSagas.ts(6 hunks)app/client/src/sagas/ReplaySaga.ts(4 hunks)app/client/src/workers/Evaluation/formEval.ts(10 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/actions/evaluationActions.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/workers/Evaluation/formEval.ts
[error] 350-350: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 353-353: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (8)
app/client/src/actions/formEvaluationActions.ts (1)
23-46: LGTM!The
startFormEvaluationsfunction is well-structured with proper type definitions and clear parameter naming.app/client/src/sagas/FormEvaluationSaga.ts (2)
39-50: LGTM!The
FormEvalActionPayloadinterface is properly updated with the neweditorContextTypefield.
146-146: LGTM!Good use of the
objectKeysutility function for type-safe key iteration.app/client/src/sagas/ReplaySaga.ts (1)
107-107: LGTM!Good use of type assertion with
objectKeysutility function.app/client/src/sagas/QueryPaneSagas.ts (2)
346-346: LGTM!Good use of default value
ActionParentEntityType.PAGEforeditorContextType.
283-283: LGTM!Good use of the
objectKeysutility function for type-safe key iteration.app/client/src/workers/Evaluation/formEval.ts (2)
18-22: LGTM! Good practice using the objectKeys utility.The introduction of
objectKeysutility and proper type imports enhances type safety.
601-601: Verify the default value for editorContextType.The code uses
ActionParentEntityType.PAGEas a default value. Ensure this aligns with the PR objectives which mention that valid values includeWORKFLOWS,PAGE, orMODULE.✅ Verification successful
🌐 Web query:
What are the valid values for editorContextType in Appsmith?💡 Result:
Based on the search results provided, there's no explicit mention of
editorContextTypein Appsmith's documentation. However, analyzing related context properties reveals that the closest relevant concept is Appsmith'smodeproperty within its JavaScript context object[4][6][9]. This property indicates the current execution environment with three possible values:Valid modes:
- EDITOR - When working in Appsmith's development interface
- BUILDER - Alternative terminology for development environment (synonymous with EDITOR in some contexts)
- DEPLOYED - When viewing the published/running application
These modes are accessible via:
appsmith.mode // Returns current mode valueKey contextual usage examples:
- Controlling widget visibility based on environment[6]:
{{ appsmith.mode === 'EDITOR' ? 'visible' : 'hidden' }}
- Adapting component behavior for different environments[4]:
function ThemeButton() { const [themeStyles, setThemeStyles] = React.useState({}); React.useEffect(() => { if (appsmith.mode === 'DEPLOYED') { // Apply production theme logic } }, []); }While "editorContextType" isn't directly defined in the documentation, this mode system serves as Appsmith's primary context differentiation mechanism between development and runtime environments[9]. For specific implementation details beyond these general modes, you'd need to consult internal framework documentation or source code references not included in these search results[3].
Citations:
- 1: https://docs.appsmith.com/reference/widgets/table/editable
- 2: https://docs.appsmith.com/core-concepts/building-ui/designing-an-application/app-theming
- 3: https://github.com/appsmithorg/appsmith/blob/release/contributions/AppsmithWidgetDevelopmentGuide.md
- 4: https://docs.appsmith.com/reference/widgets/custom
- 5: https://docs.appsmith.com/build-apps/how-to-guides/update-list-data
- 6: https://www.youtube.com/watch?v=5Z65l_PssEQ
- 7: https://docs.appsmith.com/reference/widgets/table/column-settings
- 8: https://docs.appsmith.com/reference/widgets/select
- 9: https://community.appsmith.com/content/guide/intro-javascript-appsmith
- 10: https://docs.appsmith.com/reference/appsmith-framework/widget-actions/store-value
I'll start by locating the definition of
ActionParentEntityTypeto verify its valid values. Let’s run a search for its enum definition in the codebase.
🏁 Script executed:
#!/bin/bash # Search for the definition of ActionParentEntityType to check its valid values. rg "enum ActionParentEntityType" -n .Length of output: 39
I'll search within the file itself to catch any hints of an inline definition or usage pattern.
🏁 Script executed:
#!/bin/bash # Search for occurrences of ActionParentEntityType in formEval.ts rg "ActionParentEntityType" -n app/client/src/workers/Evaluation/formEval.tsLength of output: 384
Let's further search the repository for usages of
ActionParentEntityType.PAGEto see how it's defined or used elsewhere.
🏁 Script executed:
#!/bin/bash # Search for where ActionParentEntityType.PAGE is referenced in the entire repository. rg "ActionParentEntityType\\.PAGE" -n .Length of output: 1863
The usages throughout the codebase consistently rely on using
ActionParentEntityType.PAGEas the default for theeditorContextType(for example, in helpers, hooks, etc.). There isn’t any indication in our search that another default (likeWORKFLOWSorMODULE) should be used. Since the PR objectives list valid options asWORKFLOWS,PAGE, orMODULEand the rest of the codebase usesPAGEas the default fallback, the default value appears consistent with the intended PR behavior.Final verdict: The default value is appropriately set to
ActionParentEntityType.PAGE.
## Description Added an extra condition for form evaluation. This allows server to render different components for application or any other editors. Form configs can now use this as a conditional to show or hide components `editorContextType === "PAGE"`. Valid values are `WORKFLOWS`, `PAGE` or `MODULE`. Fixes appsmithorg#37735 ## Automation /ok-to-test tags="@tag.Sanity, @tag.IDE" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13294166183> > Commit: 6ab7333 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13294166183&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.IDE` > Spec: > <hr>Wed, 12 Feb 2025 21:15:04 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced context-aware evaluation actions to improve dynamic form interactions. - **Refactor** - Streamlined the form evaluation workflow and standardized key extraction for enhanced consistency and reliability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Added an extra condition for form evaluation. This allows server to render different components for application or any other editors.
Form configs can now use this as a conditional to show or hide components
editorContextType === "PAGE". Valid values areWORKFLOWS,PAGEorMODULE.Fixes #37735
Automation
/ok-to-test tags="@tag.Sanity, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13294166183
Commit: 6ab7333
Cypress dashboard.
Tags:
@tag.Sanity, @tag.IDESpec:
Wed, 12 Feb 2025 21:15:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor