feat: Add currentPage, workspace, application name to appsmith context #38114
Conversation
WalkthroughThe pull request introduces new properties to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12358867744. |
![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 (1)
app/client/src/selectors/dataTreeSelectors.ts (1)
168-168: Remove development commentThis commented code appears to be a development leftover and should be removed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/entities/DataTree/types.ts(1 hunks)app/client/src/selectors/dataTreeSelectors.ts(3 hunks)
🔇 Additional comments (4)
app/client/src/selectors/dataTreeSelectors.ts (3)
44-46: LGTM! New imports are properly organized
The new imports for current application, workspace, and page name are correctly added and logically grouped with other selector imports.
143-156: LGTM! Selector signature updated correctly
The getUnevaluatedDataTree selector has been properly extended to include the new context parameters.
176-178:
Fix getCurrentPageName parameter usage
There's a bug in the property assignment. The getCurrentPageName parameter is being used directly instead of its value.
Apply this fix:
- currentPageName: getCurrentPageName,
+ currentPageName: getCurrentPageName,
workspaceName: currentWorkspace.name,
appName: currentApplication?.name,Likely invalid or redundant comment.
app/client/src/ce/entities/DataTree/types.ts (1)
192-194: LGTM! AppsmithEntity interface properly extended
The new properties are correctly typed and align with the requirements to enhance the appsmith context.
|
Deploy-Preview-URL: https://ce-38114.dp.appsmith.com |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/selectors/dataTreeSelectors.ts (3)
136-143: Consider improving edge case handling and documentationWhile the implementation is functional, consider:
- Adding a default value when no matching page is found
- Expanding the internal use comment to explain the cyclic dependency issue
// * This is only for internal use to avoid cyclic dependency issue + // * Used internally by getUnevaluatedDataTree to avoid circular dependencies + // * between page and data tree selectors const getPageListState = (state: AppState) => state.entities.pageList; const getCurrentPageName = createSelector( getPageListState, (pageList: PageListReduxState) => pageList.pages.find((page) => page.pageId === pageList.currentPageId) - ?.pageName, + ?.pageName ?? "Unknown Page", );
177-177: Remove development commentThe commented code appears to be a development leftover and should be removed.
- // const { currentPageName, workspaceName, appName } = someObj;
Line range hint
152-187: Improve parameter naming and null safetyA few suggestions for improvement:
- Rename the
getCurrentPageNameparameter to avoid confusion with the selector- Add null checks for workspace name for consistency
getCurrentApplication, getCurrentPageName, ( actions, jsActions, widgets, metaWidgets, appData, theme, currentWorkspace, currentApplication, - getCurrentPageName, + currentPageName, ) => { // ... dataTree.appsmith = { ...appData, store: appData.store, theme, - currentPageName: getCurrentPageName, + currentPageName, - workspaceName: currentWorkspace.name, + workspaceName: currentWorkspace?.name ?? "Unknown Workspace", appName: currentApplication?.name, } as AppsmithEntity;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/selectors/dataTreeSelectors.ts(3 hunks)
🔇 Additional comments (2)
app/client/src/selectors/dataTreeSelectors.ts (2)
44-46: LGTM!
The new imports are properly organized and necessary for the enhanced appsmith context functionality.
185-187: Verify appsmith context usage
Let's verify that all consumers of the appsmith context are prepared for these new properties.
✅ Verification successful
Let me search for actual usages and type definitions of these properties to verify their impact on the codebase.
Let me try a broader search to find any references to these properties and their related selectors.
Changes to appsmith context properties are properly handled
Based on the codebase analysis:
- The properties
currentPageName,workspaceName, andappNameare properly defined in the AppsmithEntity interface - These properties are used consistently across the application for analytics, page navigation, and UI display
- The changes are additive and existing consumers are already prepared for these properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of appsmith context properties
rg -l "appsmith\.(currentPageName|workspaceName|appName)"
# Search for type checks or property access
ast-grep --pattern 'appsmith.$_'
Length of output: 86392
Script:
#!/bin/bash
# Search for usages of appsmith context properties in TypeScript files
rg -t ts "appsmith\.(currentPageName|workspaceName|appName)" -A 2 -B 2
# Search for type definitions of appsmith context
ast-grep --pattern 'interface $_Context {
$$$
currentPageName?: $_
workspaceName?: $_
appName?: $_
$$$
}'
Length of output: 190
Script:
#!/bin/bash
# Search for any references to these properties in the codebase
rg -g '*.{ts,tsx,js,jsx}' "currentPageName|workspaceName|appName" -A 2 -B 2
# Search for type definitions or interfaces that might contain these properties
ast-grep --pattern 'type $_ = {
$$$
currentPageName: $_
$$$
}'
# Look for any usage of these properties in selectors
rg -g '*.ts' "getCurrentPageName|getWorkspaceName|getAppName" -A 2 -B 2
Length of output: 73160
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12367779139. |
sneha122
requested review from
AmanAgarwal041
and removed request for
ApekshaBhosale and
sneha122
|
Deploy-Preview-URL: https://ce-38114.dp.appsmith.com |
sneha122
added
ok-to-test
|
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. |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/selectors/dataTreeSelectors.ts (1)
137-138: Enhance the internal commentThe current comment could be more descriptive about why this selector needs to be internal.
-// * This is only for internal use to avoid cyclic dependency issue +// Internal selector to prevent cyclic dependency between page and datatree modules
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/entities/DataTree/types.ts(1 hunks)app/client/src/selectors/dataTreeSelectors.ts(3 hunks)
🔇 Additional comments (2)
app/client/src/ce/entities/DataTree/types.ts (1)
193-196: LGTM! Properties added to AppsmithEntity interface
The new properties are well-typed and follow the existing naming conventions.
app/client/src/selectors/dataTreeSelectors.ts (1)
153-167: LGTM! Selector signature updated correctly
The new parameters are properly added and destructured in the selector.
| currentPageName: getCurrentPageName, | ||
| workspaceName: currentWorkspace.name, | ||
| appName: currentApplication?.name, | ||
| currentEnvName: currentEnvironmentName, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for workspace and environment properties
While currentApplication?.name uses optional chaining, currentWorkspace.name and currentEnvironmentName are accessed directly. Consider adding null checks for consistency and safety.
- currentPageName: getCurrentPageName,
- workspaceName: currentWorkspace.name,
- appName: currentApplication?.name,
- currentEnvName: currentEnvironmentName,
+ currentPageName: getCurrentPageName ?? "",
+ workspaceName: currentWorkspace?.name ?? "",
+ appName: currentApplication?.name ?? "",
+ currentEnvName: currentEnvironmentName ?? "",📝 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.
| currentPageName: getCurrentPageName, | |
| workspaceName: currentWorkspace.name, | |
| appName: currentApplication?.name, | |
| currentEnvName: currentEnvironmentName, | |
| currentPageName: getCurrentPageName ?? "", | |
| workspaceName: currentWorkspace?.name ?? "", | |
| appName: currentApplication?.name ?? "", | |
| currentEnvName: currentEnvironmentName ?? "", |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12461874669. |
|
Deploy-Preview-URL: https://ce-38114.dp.appsmith.com |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts (1)
Line range hint
273-273: Replace XPath usage with data- attribute or a custom locator*
According to the guidelines, avoid using XPath and prefer data-* attributes for reliable and maintainable selectors.Example minimal fix:
-cy.xpath(documentInputSelector) +cy.get("[data-cy=document-input]")
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts (1)
Line range hint
269-272: Avoid using cy.wait for fixed time intervals
Using cy.wait(200) introduces flakiness and can slow down tests. It's best to wait on a specific event or condition instead.A possible approach is to wait on an element state:
cy.get(selector).should("be.visible")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts(2 hunks)app/client/src/ce/entities/DataTree/types.ts(1 hunks)app/client/src/selectors/dataTreeSelectors.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/ce/entities/DataTree/types.ts
- app/client/src/selectors/dataTreeSelectors.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts (2)
301-301: Recognition of new property 'appName'
This check aligns with the new property added to the appsmith context. The assertion confirms the autocomplete suggestion for "appName".
316-316: Correct validation for 'appName' hint
Verifying "appName" appears again ensures consistency in multiple binding scenarios. Looks good.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12463121010. |
|
Deploy-Preview-URL: https://ce-38114.dp.appsmith.com |
#38114) ## Description Add currentPageName, AppName and workspace name to appsmith context object Fixes #38046 ## Automation /ok-to-test tags="@tag.JS" ### 🔍 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/12462985063> > Commit: eec4292 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12462985063&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Mon, 23 Dec 2024 09:14:06 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** - Enhanced `AppsmithEntity` interface with new properties: `currentPageName`, `workspaceName`, and `appName`. - Updated `getUnevaluatedDataTree` selector to accept additional parameters, providing more contextual information about the current application state. - Introduced a new internal selector to retrieve the current page's name. - **Bug Fixes** - Adjusted expected hint text in autocomplete tests for JavaScript objects to align with updated behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
Description
Add currentPageName, AppName and workspace name to appsmith context object
Fixes #38046
Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12462985063
Commit: eec4292
Cypress dashboard.
Tags:
@tag.JSSpec:
Mon, 23 Dec 2024 09:14:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
AppsmithEntityinterface with new properties:currentPageName,workspaceName, andappName.getUnevaluatedDataTreeselector to accept additional parameters, providing more contextual information about the current application state.Bug Fixes