Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handles button binding during buildingblock pasting #33674

Merged
merged 6 commits into from
May 29, 2024

Conversation

rahulbarwal
Copy link
Contributor

@rahulbarwal rahulbarwal commented May 23, 2024

Description

This pull request adds the handleButtonDynamicTriggerPathList function to the BuildingBlockAdditionSagas.ts file. This function is responsible for handling the dynamic trigger path list for button widgets. It iterates over the widgetNameMap and updates the dynamicTriggerPathList of each button widget with the newWidgetName.

Additionally, unit tests have been added to ensure the correct functionality of the handleButtonDynamicTriggerPathList function. This change improves the functionality of button widgets when pasting building block widgets.

Fixes #33658
or
Fixes Issue URL

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.Widget"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9268894716
Commit: a675a5c
Cypress dashboard url: Click here!

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

This commit adds the handleButtonDynamicTriggerPathList function to the BuildingBlockAdditionSagas.ts file. This function is responsible for handling the dynamic trigger path list for button widgets. It iterates over the widgetNameMap and updates the dynamicTriggerPathList of each button widget with the newWidgetName. This change improves the functionality of button widgets when pasting building block widgets.
@rahulbarwal rahulbarwal self-assigned this May 23, 2024
@rahulbarwal rahulbarwal added Templates Product Issues related to Templates Building blocks Building blocks on cavas, on templates listing or drag and drop of building blocks. labels May 23, 2024
@rahulbarwal rahulbarwal requested a review from jacquesikot May 23, 2024 04:35
Copy link
Contributor

coderabbitai bot commented May 23, 2024

Walkthrough

The recent changes focus on introducing a new function, handleButtonDynamicTriggerPathList, to manage dynamic trigger paths for button widgets during the process of pasting building block widgets. This function aims to ensure that widget names within dynamic trigger paths are updated correctly, addressing the binding issues that arise when multiple Insert data building blocks are utilized.

Changes

Files Change Summary
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts Added handleButtonDynamicTriggerPathList function to manage button widget dynamic trigger paths.
app/client/src/sagas/PasteWidgetUtils/PasteWidgetUtils.test.ts Added handleButtonDynamicTriggerPathList function along with its corresponding test cases. Imported klona from "klona".
app/client/src/sagas/PasteWidgetUtils/index.ts Introduced handleButtonDynamicTriggerPathList function to update widget names within dynamic trigger paths.
app/client/src/sagas/PasteWidgetUtils/utils.ts Updated handleWidgetUpdate function signature to include isParentSelected parameter.
app/client/src/selectors/widgetSelectors.ts Modified isWidgetSelected function to accept an optional widgetId parameter.

Assessment against linked issues

Objective (Issue #33658) Addressed Explanation
Ensure UI bindings update correctly when pasting building blocks

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added Bug Something isn't working Low An issue that is neither critical nor breaks a user flow Medium Issues that frustrate users due to poor UX and removed Bug Something isn't working labels May 23, 2024
@rahulbarwal rahulbarwal changed the title "Add handleButtonDynamicTriggerPathList function and unit tests" fix: Handles button binding during buildingblock pasting May 23, 2024
@github-actions github-actions bot added the Bug Something isn't working label May 23, 2024
This commit adds the handleButtonDynamicTriggerPathList function to the BuildingBlockAdditionSagas.ts file. This function is responsible for handling the dynamic trigger path list for button widgets. It iterates over the widgetNameMap and updates the dynamicTriggerPathList of each button widget with the newWidgetName. This change improves the functionality of button widgets when pasting building block widgets.
@rahulbarwal rahulbarwal requested review from sharat87, ankitakinger and a team as code owners May 24, 2024 04:58
@rahulbarwal rahulbarwal requested review from a team and marks0351 and removed request for a team May 24, 2024 04:58
@rahulbarwal rahulbarwal marked this pull request as draft May 24, 2024 04:59
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Outside diff range and nitpick comments (53)
app/client/src/layoutSystems/anvil/editor/hooks/useAnvilWidgetHover.ts (1)

Line range hint 25-25: Consider specifying a more appropriate type than any for the event parameter in handleMouseOver.

- (e: any) => {
+ (e: React.MouseEvent<HTMLDivElement>) => {
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/derived.js (4)

Line range hint 4-4: Declare variables hasValidValue and value separately for better readability.

- let hasValidValue, value;
+ let hasValidValue;
+ let value;

Line range hint 49-51: Remove unnecessary else clauses to simplify the control flow.

- } else {
-   return hasValidValue;
- }
+ return hasValidValue;

Also applies to: 85-87


Line range hint 74-74: Use Number.parseFloat instead of the global parseFloat for better clarity and consistency.

- const parsed = parseFloat(
+ const parsed = Number.parseFloat(

Line range hint 81-81: Correct the assignment to a constant variable.

- parsed = undefined;
+ let parsed = parseFloat(
+   text
+     .replace(new RegExp(`[${getLocaleThousandSeparator()}]`, "g"), "")
+     .replace(new RegExp(`[${getLocaleDecimalSeperator()}]`), "."),
+ );
+ if (Number.isNaN(parsed)) {
+   parsed = undefined;
+ }
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (1)

Line range hint 34-34: Remove unnecessary double negation.

- description: !!data.isBeta ? <Tag isClosable={false}>Beta</Tag> : "",
+ description: data.isBeta ? <Tag isClosable={false}>Beta</Tag> : "",
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/InstanceAdminControllerCE.java (1)

Line range hint 1-1: Ensure proper package organization and naming conventions in Java.

Consider organizing your Java packages by feature rather than by layer to improve modularity and scalability.

app/client/src/ce/entities/IDE/constants.ts (1)

Line range hint 150-150: Specify a more appropriate type than any.

- component: ComponentType<any>;
+ component: ComponentType<SpecificProps>; // Replace `SpecificProps` with the actual prop type
app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/AnvilWidgetNameComponent.tsx (1)

Line range hint 61-61: Ensure all dependencies are specified in useCallback hooks.

- const handleSelectParent = useCallback(() => {
+ const handleSelectParent = useCallback(() => {
+ }, [parentId, selectWidget]); // Added missing dependencies

- const handleSelectWidget = useCallback(() => {
+ const handleSelectWidget = useCallback(() => {
+ }, [props.widgetId, selectWidget]); // Added missing dependencies

- const handleDebugClick = useCallback(() => {
+ const handleDebugClick = useCallback(() => {
+ }, [props.widgetId, dispatch]); // Added missing dependencies

Also applies to: 65-65, 69-69

app/client/src/widgets/wds/WDSInputWidget/widget/helper.ts (3)

Line range hint 29-29: Replace isNaN with Number.isNaN for safer type checking.

- if (isNaN(parsedText)) return null;
+ if (Number.isNaN(parsedText)) return null;

Line range hint 110-110: Replace isNaN with Number.isNaN in numeric validation.

- if (rawText !== "" && isNaN(Number(rawText))) {
+ if (rawText !== "" && Number.isNaN(Number(rawText))) {

Line range hint 134-134: Specify a more explicit type instead of any for propertyValue.

- propertyValue: any,
+ propertyValue: string | number | boolean, // Adjust based on expected types
app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/SplitButton.tsx (2)

Line range hint 122-127: Add keyboard accessibility to clickable elements.

+ onKeyUp={props.leftToggle.onClick} // Add appropriate keyboard event handlers
+ onKeyUp={props.rightToggle.onClick} // Add appropriate keyboard event handlers

Also applies to: 133-138


Line range hint 131-131: Specify the type attribute for the button element.

+ type="button", // Specify button type to avoid default form submission behavior
app/client/src/layoutSystems/anvil/editor/AnvilWidgetName/index.tsx (1)

Line range hint 1-1: The default import is only used as a type, consider using import type.

- import React, { useCallback, useEffect, useState } from "react";
+ import type React from "react";
+ import { useCallback, useEffect, useState } from "react";
app/client/src/pages/Editor/Canvas.tsx (1)

Line range hint 43-43: Avoid using template literals where not necessary.

- background: ${({ background }) => background};
+ background: ${props => props.background};

- className={`relative t--canvas-artboard ${paddingBottomClass} transition-all duration-400  ${marginHorizontalClass} ${getViewportClassName(
-            canvasWidth,
-          )}`}
+ className={"relative t--canvas-artboard " + paddingBottomClass + " transition-all duration-400 " + marginHorizontalClass + " " + getViewportClassName(canvasWidth)}

Also applies to: 87-87, 88-88

app/client/src/ce/api/UserApi.tsx (2)

Line range hint 86-254: Consider refactoring UserApi to a module with exported functions if no instance-specific data is used.

- export class UserApi extends Api {
+ export const UserApi = {

Line range hint 122-122: Use template literals for string concatenation to improve readability.

- return Api.get(UserApi.usersURL + "/" + request.id);
+ return Api.get(`${UserApi.usersURL}/${request.id}`);

- return Api.put(UserApi.leaveWorkspaceURL + "/" + request.workspaceId);
+ return Api.put(`${UserApi.leaveWorkspaceURL}/${request.workspaceId}`);

Also applies to: 206-206

app/client/src/ce/pages/AdminSettings/config/general.tsx (2)

Line range hint 150-150: Specify a more appropriate type instead of any for better type safety.

- } as any;
+ } as SpecificType; // Replace `SpecificType` with the actual type

Line range hint 165-171: Remove unnecessary else clauses to simplify control flow.

- } else {
+ }

Also applies to: 169-171

app/client/src/widgets/wds/WDSInputWidget/widget/index.tsx (1)

Line range hint 139-139: Use Number.isNaN instead of isNaN for type-safe checking.

- if (isNumber(newValue) && !isNaN(newValue)) {
+ if (isNumber(newValue) && !Number.isNaN(newValue)) {
app/client/src/selectors/widgetSelectors.ts (2)

Line range hint 90-90: Avoid using hasOwnProperty directly due to potential issues with prototype chain.

- if (canvasWidgets.hasOwnProperty(widgetId)) {
+ if (Object.prototype.hasOwnProperty.call(canvasWidgets, widgetId)) {

Also applies to: 92-92


Line range hint 165-165: Remove unnecessary continue statement to simplify the loop logic.

- continue;
app/client/src/widgets/wds/WDSCurrencyInputWidget/widget/index.tsx (4)

Line range hint 66-66: Avoid using super in a static context as it can lead to confusion.

- return super.getPropertyPaneContentConfig();
+ return WDSBaseInputWidget.getPropertyPaneContentConfig();

Line range hint 77-77: Specify a type other than any to ensure type safety.

- } as any;
+ } as SpecificType; // Replace `SpecificType` with the actual type

Also applies to: 90-90, 101-101, 117-117


Line range hint 107-107: Avoid using super in a static context as it can lead to confusion.

- return super.getMetaPropertiesMap();
+ return WDSBaseInputWidget.getMetaPropertiesMap();

Also applies to: 118-118, 125-125


Line range hint 167-167: Use optional chaining to simplify the code.

- let formattedValue = "";
- if (value && value.includes(decimalSeperator)) {
+ let formattedValue = value?.includes(decimalSeperator) ? limitDecimalValue(this.props.decimals, value) : value;
app/client/src/ce/utils/AnalyticsUtil.tsx (1)

Line range hint 63-321: Consider refactoring the class to avoid only static members, which can be a code smell.

// Consider converting static methods to instance methods or moving them to a more appropriate utility class.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RestAPIActivateUtils.java (1)

Line range hint 74-98: Refactor the method to handle API calls more efficiently and handle errors more gracefully.

// Consider breaking down this method into smaller, more manageable methods that handle specific parts of the API call process.
app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx (2)

481-481: Ensure consistency in test descriptions.

The test descriptions should consistently describe what the test is doing. For example, "Renders Postgres add routes in Split Screen" should clearly state what is expected to happen in the test. Consider rephrasing to include more details about the test's purpose and expected outcomes.


[!TIP]
Codebase Verification

The review comment is correct. The QueryRender.test.tsx file uses PostgresFactory instead of GoogleSheetFactory for setting up Google Sheets routes tests. This appears to be a copy-paste error.

  • File: app/client/src/pages/Editor/IDE/EditorPane/Query/QueryRender.test.tsx
    • Lines: 632-645
    • Issue: PostgresFactory is used instead of GoogleSheetFactory.
Analysis chain

Line range hint 635-677: Review the use of factory in test setup.

The use of PostgresFactory for creating a GoogleSheetFactory instance seems incorrect. This could be a copy-paste error. Please verify if the correct factory is being used for setting up Google Sheets routes tests.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify correct factory usage in Google Sheets routes tests.

# Test: Search for incorrect factory usage. Expect: No occurrences of PostgresFactory in Google Sheets tests.
rg --type tsx --files-with-matches 'GoogleSheetFactory' | xargs rg 'PostgresFactory'

Length of output: 1399



Script:

#!/bin/bash
# Description: Search for the definition and usage of GoogleSheetFactory in the codebase.

# Find the definition of GoogleSheetFactory
rg --type ts 'GoogleSheetFactory'

# Find all instances where GoogleSheetFactory is used
rg --type tsx 'GoogleSheetFactory'

Length of output: 732

app/client/src/ce/selectors/entitiesSelector.ts (20)

Line range hint 165-165: Consider using optional chaining for safer access.

- const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);
+ const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);

Line range hint 170-172: This else clause can be omitted for cleaner code, as previous branches return early.

- if (!plugin) return undefined;
- return plugin.name;
+ return plugin?.name;

Line range hint 382-382: Specify a more precise type instead of any to enhance type safety.

Please replace any with a more specific type if possible.


Line range hint 400-400: Avoid using any type to ensure type safety.

Consider specifying a more detailed type for better type checking.


Line range hint 404-404: Type any should be replaced with a more specific type.

Using any can lead to runtime errors due to lack of type safety. Please specify a more precise type.


Line range hint 554-556: Prefer using for...of for iterating over arrays for better readability and performance.

- jsCollections.forEach((collection) => {
+ for (const collection of jsCollections) {

Line range hint 563-565: Use for...of instead of forEach for array iteration to improve clarity and performance.

- allActions.forEach((action) => {
+ for (const action of allActions) {

Line range hint 571-571: Replace any with a more specific type to improve code quality and safety.

Please use a more specific type instead of any.


Line range hint 573-575: Consider using for...of for array iteration for better performance and readability.

- jsCollections.forEach((collection) => {
+ for (const collection of jsCollections) {

Line range hint 581-581: Avoid using any type to ensure type safety.

Consider specifying a more detailed type for better type checking.


Line range hint 583-585: Prefer using for...of for iterating over arrays for better readability and performance.

- jsCollections.forEach((collection) => {
+ for (const collection of jsCollections) {

Line range hint 595-597: Use for...of instead of forEach for array iteration to improve clarity and performance.

- plugins.forEach((plugin) => {
+ for (const plugin of plugins) {

Line range hint 724-726: Consider using for...of for array iteration for better performance and readability.

- plugins.forEach((plugin) => {
+ for (const plugin of plugins) {

Line range hint 751-751: Consider using optional chaining for safer access.

- const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);
+ const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);

Line range hint 766-766: Consider using optional chaining for safer access.

- const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);
+ const plugin = state.entities.plugins.list.find((plugin) => plugin.id === datasource?.pluginId);

Line range hint 837-837: Specify a more precise type instead of any to enhance type safety.

Please replace any with a more specific type if possible.


Line range hint 838-838: Avoid using any type to ensure type safety.

Consider specifying a more detailed type for better type checking.


Line range hint 840-840: This variable implicitly has the any type, which could lead to runtime errors.

Please specify a type for this variable to enhance code safety and clarity.


Line range hint 858-858: Type any should be replaced with a more specific type.

Using any can lead to runtime errors due to lack of type safety. Please specify a more precise type.


Line range hint 875-875: Replace any with a more specific type to improve code quality and safety.

Please use a more specific type instead of any.

app/client/src/ce/constants/messages.ts (2)

Line range hint 4-4: Consider specifying a more explicit type instead of any for better type safety and code maintainability.

- export function createMessage(
-   format: (...strArgs: any[]) => string,
-   ...args: any[]
+ export function createMessage<T>(
+   format: (...strArgs: T[]) => string,
+   ...args: T[]
) {

Line range hint 16-47: Avoid using template literals when not performing interpolation or needing special character handling, as it can lead to unnecessary computation and reduced clarity.

- export const APPSMITH_DISPLAY_VERSION = (edition: string, version: string) =>
-   `Appsmith ${edition} ${version}`;
+ export const APPSMITH_DISPLAY_VERSION = (edition: string, version: string) =>
+   "Appsmith " + edition + " " + version;

Apply this change to all constants where template literals are used unnecessarily.

.onErrorResume(error -> {
boolean isBodySentWithApiRequest = requestBodyObj == null ? false : true;
errorResult.setRequest(requestCaptureFilter.populateRequestFields(
actionExecutionRequest, isBodySentWithApiRequest));
actionExecutionRequest, isBodySentWithApiRequest, datasourceConfiguration));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhance error handling by checking the type of error and providing more specific messages.

if (!(error instanceof AppsmithPluginException)) {
    error = new AppsmithPluginException(
        GraphQLPluginError.QUERY_EXECUTION_FAILED,
        GraphQLErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG,
        error);
}
errorResult.setErrorInfo(error);

Consider refining this block to handle different types of exceptions more granularly, potentially logging or handling specific cases differently.

Comment on lines 149 to 150
-p 5432:5432 -p 28017:27017 -p 25:25 -p 4200:4200 -p 5000:5000 -p 3001:3000 -p 6001:6001 -p 8001:8000 --privileged --pid=host --ipc=host --volume /:/host -v ~/git-server/keys:/git-server/keys \
"appsmith/test-event-driver:${ted_tag:-latest}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust the Docker run command to ensure proper resource allocation and security.

The Docker run command uses --privileged which grants the container full access to the host, potentially leading to security risks. Consider removing this flag if not necessary or ensure that its usage is justified and secure. Additionally, the port mappings have been adjusted, ensure these changes are intended and do not expose unnecessary ports.

// On leaving a widget, reset the focused widget
focusWidget && focusWidget();
}, [focusWidget]);

// Effect hook to add and remove mouseover and mouseleave event listeners
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure all dependencies are specified in the useEffect hook to avoid potential bugs.

- }, [handleMouseOver]);
+ }, [handleMouseOver, ref.current]);

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.

Suggested change
useEffect(() => {
useEffect(() => {

@@ -1,20 +1,8 @@
import type { ForwardedRef, CSSProperties } from "react";
import React, { forwardRef } from "react";
import React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imports are only used as types, consider using import type.

- import React from "react";
- import styled from "styled-components";
+ import type React from "react";
+ import type styled from "styled-components";

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.

Suggested change
import React from "react";
import type React from "react";
import type styled from "styled-components";

@@ -77,7 +85,13 @@ export function AnvilWidgetName(props: {

let cleanup = () => {};
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add missing dependencies to the useEffect hook.

- }, [nameComponentState, widgetElement, widgetNameComponent]);
+ }, [nameComponentState, widgetElement, widgetNameComponent, cleanup, widgetsEditorElement]);

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.

Suggested change
useEffect(() => {
useEffect(() => {
}, [nameComponentState, widgetElement, widgetNameComponent, cleanup, widgetsEditorElement]);

try {
if (isFocused) {
const text = this.props.text || "";
const text = this.props.parsedText || "";
const deFormattedValue = text.replace(
new RegExp("\\" + getLocaleThousandSeparator(), "g"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer template literals over string concatenation.

- const deFormattedValue = text.replace(new RegExp("\\" + getLocaleThousandSeparator(), "g"), "");
+ const deFormattedValue = text.replace(new RegExp(`\\${getLocaleThousandSeparator()}`, "g"), "");

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.

Suggested change
new RegExp("\\" + getLocaleThousandSeparator(), "g"),
new RegExp(`\\${getLocaleThousandSeparator()}`, "g"),

@@ -189,14 +189,18 @@
};

onFocusChange = (isFocused?: boolean) => {
// We don't want to deformat or the text or trigger
// any event on focus if the widget is read only
if (Boolean(this.props.isReadOnly)) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid redundant Boolean conversion.

- if (Boolean(this.props.isReadOnly)) return;
+ if (this.props.isReadOnly) return;

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.

Suggested change
if (Boolean(this.props.isReadOnly)) return;
if (this.props.isReadOnly) return;

Comment on lines 59 to 91
private URI createURIWithMaskedQueryParam(URI uriToMask, String queryParamKeyToMask) {

String query = uriToMask.getQuery();
if (query == null) {
return uriToMask;
}
String[] queryParams = query.split("&");
StringJoiner newQuery = new StringJoiner("&");
for (String queryParam : queryParams) {
String[] keyValuePair = queryParam.split("=");

if (queryParamKeyToMask.equals(keyValuePair[0])) {
// fix when value is not present
if (keyValuePair.length > 1) {
keyValuePair[1] = MASKED_VALUE;
}
}
newQuery.add(keyValuePair[0] + "=" + keyValuePair[1]);
}

try {
return new URI(
uriToMask.getScheme(),
uriToMask.getUserInfo(),
uriToMask.getHost(),
uriToMask.getPort(),
uriToMask.getPath(),
newQuery.toString(),
uriToMask.getRawFragment());
} catch (URISyntaxException e) {
return uriToMask;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the method to handle query parameters more efficiently and safely.

private URI createURIWithMaskedQueryParam(URI uriToMask, String queryParamKeyToMask) {
    UriComponentsBuilder builder = UriComponentsBuilder.fromUri(uriToMask);
    MultiValueMap<String, String> queryParams = builder.build().getQueryParams();

    queryParams.forEach((key, values) -> {
        if (queryParamKeyToMask.equals(key)) {
            values.replaceAll(value -> MASKED_VALUE);
        }
    });

    return builder.replaceQueryParams(queryParams).build().toUri();
}

@@ -245,7 +245,7 @@ class AnalyticsUtil {
? {
id: AnalyticsUtil.cachedAnonymoustId,
email: userData.email,
appsmithVersion: `Appsmith ${appVersion.edition} ${appVersion.id}`,
version: `Appsmith ${appVersion.edition} ${appVersion.id}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use template literals for string concatenation.

version: `Appsmith ${appVersion.edition} ${appVersion.id}`,

Comment on lines 29 to 45
const datasources = useSelector(getDatasources);
const getConditionalIconAndTooltip = (
type?: SideButtonType,
conditionTooltip?: string,
) => {
switch (type) {
case SideButtonType.DATSOURCE:
if (datasources.length === 0)
return {
conditionIcon: "warning",
tooltip: conditionTooltip,
};
return {};
default:
return {};
}
};
Copy link
Contributor

@coderabbitai coderabbitai bot May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the getConditionalIconAndTooltip function to improve readability and maintainability.

- switch (type) {
-   case SideButtonType.DATSOURCE:
-     if (datasources.length === 0)
-       return {
-         conditionIcon: "warning",
-         tooltip: conditionTooltip,
-       };
-     return {};
-   default:
-     return {};
- }
+ if (type === SideButtonType.DATSOURCE && datasources.length === 0) {
+   return {
+     conditionIcon: "warning",
+     tooltip: conditionTooltip,
+   };
+ }
+ return {};

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.

Suggested change
const datasources = useSelector(getDatasources);
const getConditionalIconAndTooltip = (
type?: SideButtonType,
conditionTooltip?: string,
) => {
switch (type) {
case SideButtonType.DATSOURCE:
if (datasources.length === 0)
return {
conditionIcon: "warning",
tooltip: conditionTooltip,
};
return {};
default:
return {};
}
};
const datasources = useSelector(getDatasources);
const getConditionalIconAndTooltip = (
type?: SideButtonType,
conditionTooltip?: string,
) => {
if (type === SideButtonType.DATSOURCE && datasources.length === 0) {
return {
conditionIcon: "warning",
tooltip: conditionTooltip,
};
}
return {};
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment does make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marks0351, thank you for your feedback! I'm glad you found the suggestion helpful. If you have any more questions or need further assistance as you apply the changes, feel free to reach out. Happy coding! 🚀


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

@@ -4,12 +4,12 @@ export const autocompleteConfig = {
"!doc":
"An input text field is used to capture a currency value. Inputs are used in forms and can have custom validations.",
"!url": "https://docs.appsmith.com/widget-reference/currency-input",
text: {
parsedText: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KelvinOm @jsartisan can you guys review these changes to wds widgets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry folks this is due to some random rebase of release on my PR.
These are not my changes. No need to review this PR, I have for now put it in draft state, but may remove it.

So sorry @marks0351

Copy link
Contributor

@marks0351 marks0351 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of the changes are of other branches. could you pls merge latest release

@ankitakinger ankitakinger removed their request for review May 24, 2024 08:07
@rahulbarwal rahulbarwal removed the request for review from sharat87 May 24, 2024 09:24
@rahulbarwal rahulbarwal removed the request for review from a team May 24, 2024 09:24
@rahulbarwal rahulbarwal added the ok-to-test Required label for CI label May 28, 2024
@rahulbarwal rahulbarwal marked this pull request as ready for review May 28, 2024 12:01
@rahulbarwal rahulbarwal merged commit bb39e4f into release May 29, 2024
42 checks passed
@rahulbarwal rahulbarwal deleted the fix/insert-data-bb-binding branch May 29, 2024 04:09
@Nikhil-Nandagopal Nikhil-Nandagopal added the Accelerators Product Issues related to app building accelerators label Aug 6, 2024
@github-actions github-actions bot added QA Needs QA attention QA Pod Issues under the QA Pod Widgets & Accelerators Pod Issues related to widgets & Accelerators labels Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accelerators Product Issues related to app building accelerators Bug Something isn't working Building blocks Building blocks on cavas, on templates listing or drag and drop of building blocks. Low An issue that is neither critical nor breaks a user flow Medium Issues that frustrate users due to poor UX ok-to-test Required label for CI QA Pod Issues under the QA Pod QA Needs QA attention Templates Product Issues related to Templates Widgets & Accelerators Pod Issues related to widgets & Accelerators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Binding issue with insert data building block
4 participants