chore: add custom widget to anvil #37878
Conversation
WalkthroughThis pull request introduces a new custom widget for a UI builder, encapsulating various functionalities including enhanced console logging, event handling, and configuration management. Key components include a custom console implementation, configuration files for widget properties, and a communication system between the widget and its parent context. Additionally, a comprehensive test suite is added to ensure the functionality of core methods. The changes also include the integration of the new widget into the existing widget collections. Changes
Possibly related issues
Suggested labels
Suggested reviewers
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
|
|
/build-deploy-preview skip-tests=true env=release |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12116526947. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12116547629. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (19)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (2)
3-6: Consider documenting the configuration structureWhile the structure is correct, adding a brief JSDoc comment describing the configuration pattern would help maintain consistency as other WDS widgets add style configurations in the future.
+/** + * Style configuration for the Custom Widget property pane. + * Defines visual customization options available in the widget's settings. + */ export const propertyPaneStyleConfig = [
8-21: LGTM: Property configuration is well-definedThe property configuration is comprehensive with appropriate validation and clear help text. Consider adding TypeScript interfaces for better type safety and documentation.
interface PropertyConfig { propertyName: string; label: string; controlType: string; fullWidth: boolean; helpText: string; isJSConvertible: boolean; isBindProperty: boolean; isTriggerProperty: boolean; isReusable: boolean; validation: { type: string; }; }app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/metaConfig.ts (2)
1-5: Consider adding TypeScript types for imported assetsAdd type declarations for SVG imports to improve type safety and IDE support.
+declare module "*.svg" { + const content: string; + export default content; +}
8-8: Consider more descriptive widget name and search tagsThe widget name "Custom" is too generic and might not be descriptive enough for users. Additionally, the search tags could be expanded to improve discoverability.
- name: "Custom", + name: "Custom Widget", // ... - searchTags: ["external"], + searchTags: ["external", "custom", "widget", "extension"],Also applies to: 14-14
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (4)
111-112: Use optional chaining for 'props.onConsole'Simplify the null check using optional chaining.
- props.onConsole && props.onConsole(message.data.type, message.data.args); + props.onConsole?.(message.data.type, message.data.args);🧰 Tools
🪛 Biome (1.9.4)
[error] 111-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
124-124: Use optional chaining for 'iframe.current'Simplify the null checks using optional chaining.
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
136-136: Use optional chaining for 'iframe.current'Simplify the null checks using optional chaining.
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
151-151: Use optional chaining for 'iframe.current'Simplify the null checks using optional chaining.
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 151-151: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js (1)
94-106: Remove unnecessary try-catch blockThe catch block rethrows the error without handling; it's redundant.
- try { postMessageQueue.push({ type, data, }); scheduleMicrotaskToflushPostMessageQueue(); - } catch (e) { - throw e; - }🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/setterConfig.ts (1)
1-8: LGTM! Consider adding JSDoc comments for better documentation.The setter configuration is correctly structured for visibility control.
Consider adding JSDoc comments to document the purpose and usage:
+/** + * Configuration for widget property setters + * @property {Object} __setters - Collection of setter configurations + */ export const setterConfig = { __setters: { setVisibility: { path: "isVisible", type: "boolean", }, }, };app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/autocompleteConfig.ts (2)
7-13: Consider adding type validation for extraDefsToDefine.The configuration looks good, but consider adding runtime validation for
extraDefsToDefineto prevent potential type-related issues.export const autocompleteConfig = ( widget: CustomWidgetProps, extraDefsToDefine?: ExtraDef, ) => ({ isVisible: DefaultAutocompleteDefinitions.isVisible, - model: generateTypeDef(widget.model, extraDefsToDefine), + model: generateTypeDef( + widget.model, + extraDefsToDefine && typeof extraDefsToDefine === 'object' ? extraDefsToDefine : undefined + ), });
1-5: Consider grouping related imports.For better code organization, consider grouping the imports by external dependencies and internal types.
+// External dependencies import type { ExtraDef } from "utils/autocomplete/defCreatorUtils"; import { generateTypeDef } from "utils/autocomplete/defCreatorUtils"; import { DefaultAutocompleteDefinitions } from "widgets/WidgetUtils"; +// Internal types import type { CustomWidgetProps } from "../types";app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/constants.ts (1)
2-9: Consider adding type safety and JSON validationThe DEFAULT_MODEL constant could benefit from:
- Type definition for the model structure
- Runtime validation to ensure JSON string is valid
Consider this approach:
interface WidgetModel { tips: string[]; } const modelObject: WidgetModel = { tips: [ "Pass data to this widget in the default model field", "Access data in the javascript file using the appsmith.model variable", "Create events in the widget and trigger them in the javascript file using appsmith.triggerEvent('eventName')", "Access data in CSS as var(--appsmith-model-{property-name})" ] }; export const DEFAULT_MODEL = JSON.stringify(modelObject);app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts (1)
12-14: Replace magic numbers with named constantsThe UI dimensions are using magic numbers which reduces code readability.
const DEFAULT_UI_DIMENSIONS = { WIDTH: 1, HEIGHT: 2, } as const; ui: { width: DEFAULT_UI_DIMENSIONS.WIDTH, height: DEFAULT_UI_DIMENSIONS.HEIGHT, }app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (1)
12-12: Consider localizing the alert messageHardcoded strings should be localized for better maintainability and internationalization support.
onResetClick: "{{showAlert(i18n.t('widgets.custom.resetSuccess'), '');}}",app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js (1)
32-34: Enhance error event handlingConsider capturing additional error details like stack trace and error type for better debugging.
window.addEventListener("error", (event) => { - postMessage("error", [event.message]); + postMessage("error", [{ + message: event.message, + stack: event.error?.stack, + type: event.error?.name + }]); });app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts (2)
53-56: Consider bundling dependenciesUsing CDN imports can cause reliability issues. Consider bundling these dependencies with the widget.
4-95: Reduce code duplicationThe code is duplicated between
uncompiledSrcDocandsrcDoc. Consider using a build step to generate the compiled version.Also applies to: 96-184
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts (1)
61-102: Replace setTimeout with more deterministic test utilities.The test uses arbitrary timeouts which could lead to flaky tests. Consider using Jest's fake timers or async utilities.
- return new Promise((resolve) => { - setTimeout(() => { - expect(window.parent.postMessage).toHaveBeenCalledWith(/*...*/); - resolve(true); - }); - }); + jest.useFakeTimers(); + const promise = new Promise((resolve) => { + jest.advanceTimersByTime(0); + expect(window.parent.postMessage).toHaveBeenCalledWith(/*...*/); + resolve(true); + }); + jest.useRealTimers(); + return promise;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/icon.svgis excluded by!**/*.svgapp/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/thumbnail.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/reset.css(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/anvilConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/metaConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/contentConfig.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/setterConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/constants.ts(1 hunks)app/client/src/widgets/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/reset.css
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/index.ts
🧰 Additional context used
📓 Learnings (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/anvilConfig.ts (1)
Learnt from: riodeuno
PR: appsmithorg/appsmith#30351
File: app/client/src/widgets/wds/WDSModalWidget/config/anvilConfig.ts:1-6
Timestamp: 2024-11-12T08:11:43.278Z
Learning: The `anvilConfig` is optional for widgets that do not participate in the main container's layout flow, such as the modal widget in the current context.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
Learnt from: riodeuno
PR: appsmithorg/appsmith#30351
File: app/client/src/widgets/wds/WDSModalWidget/config/propertyPaneConfig/styleConfig.ts:1-3
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The `WDSModalWidget` currently has no style configurations, and they will be added in the future if needed.
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js
[error] 23-23: Unsafe usage of 'return'.
'return' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 102-102: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 111-112: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 136-136: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 151-151: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 23-23: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
[error] 64-64: 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)
🔇 Additional comments (9)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
1-1: LGTM: Import statement is correct
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/metaConfig.ts (1)
16-16: Verify airgap integration impact
The hideCard property depends on airgap status. Let's verify this behavior is consistent across the application.
✅ Verification successful
The previous search didn't provide enough context about the isAirgapped() function definition. Let's try to find more information about its usage and implementation.
Airgap integration is correctly implemented
The hideCard property's dependency on isAirgapped() is consistent with the application's airgap pattern. The function is centrally defined in airgapHelpers.tsx and imported consistently across multiple components that need airgap awareness, including other widgets and UI components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other usages of isAirgapped() to ensure consistent behavior
rg "isAirgapped\(\)" --type ts
# Look for other widget configurations that might use hideCard
ast-grep --pattern 'hideCard: $_'
Length of output: 114171
Script:
#!/bin/bash
# Let's verify the airgap integration across the codebase
# First, check for isAirgapped() function definition
rg "function isAirgapped" --type ts -A 5
# Then check for other hideCard usages with isAirgapped
rg "hideCard.*isAirgapped" --type ts
Length of output: 270
Script:
#!/bin/bash
# Let's find the implementation of isAirgapped() to understand its behavior
rg "export function isAirgapped|export const isAirgapped" --type ts -A 5
# Also check for any imports of isAirgapped to understand its source
rg "import.*isAirgapped" --type ts
Length of output: 3296
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/types.ts (1)
1-5: 'CustomWidgetProps' interface is correctly defined
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/anvilConfig.ts (1)
1-9: Verify if anvil configuration is necessary for this widget.
Based on previous learnings, anvil configuration is optional for widgets that don't participate in the main container's layout flow. Please verify if this widget requires layout integration.
If layout integration is required, the current configuration looks correct. However, consider documenting the reasoning behind isLargeWidget: true as it affects the widget's layout behavior.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (1)
11-11: Consider sourcing version from package.json
Hardcoding version numbers can lead to inconsistencies. Consider importing from package.json.
app/client/src/modules/ui-builder/ui/wds/constants.ts (1)
65-65: LGTM!
The addition follows the established naming convention and pattern.
app/client/src/widgets/index.ts (1)
90-90: LGTM! Widget registration follows the established pattern.
The WDSCustomWidget is properly imported and registered in the WDSWidgets array.
Also applies to: 191-191
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts (2)
105-124: LGTM! Comprehensive test coverage for CSS variables generation.
The test properly verifies the generation and application of CSS variables.
156-191: LGTM! Thorough test coverage for all API functions.
The tests comprehensively cover:
- Initialization and ready state
- Model changes
- UI changes
- Theme updates
- Event triggering
Also applies to: 193-243, 245-302, 304-354, 356-381, 383-412
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/metaConfig.ts
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
Show resolved
Hide resolved
...nt/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/contentConfig.tsx
Show resolved
Hide resolved
...nt/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/contentConfig.tsx
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
Show resolved
Hide resolved
| dynamicBindingPathList: [{ key: "theme" }], | ||
| dynamicTriggerPathList: [{ key: "onResetClick" }], | ||
| borderColor: Colors.GREY_5, | ||
| borderWidth: "1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert borderWidth to number type
borderWidth should be a number instead of a string for consistency and type safety.
-borderWidth: "1",
+borderWidth: 1,📝 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.
| borderWidth: "1", | |
| borderWidth: 1, |
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12117058795. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
119-155: Refactor effect hooks for better maintainabilityConsider these improvements:
- Abstract common iframe message posting logic
- Use optional chaining for iframe access
- Add complete dependency arrays
+ const postIframeMessage = useCallback((type: string, data: unknown) => { + iframe.current?.contentWindow?.postMessage({ type, ...data }, "*"); + }, []); useEffect(() => { - if (iframe.current && iframe.current.contentWindow && isIframeReady) { - iframe.current.contentWindow.postMessage( + if (isIframeReady) { + postIframeMessage( { type: EVENTS.CUSTOM_WIDGET_MODEL_CHANGE, model: props.model, - }, - "*" + } ); } - }, [props.model]); + }, [props.model, isIframeReady, postIframeMessage]);🧰 Tools
🪛 Biome (1.9.4)
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
214-230: Add JSDoc documentation to props interfaceConsider adding comprehensive documentation and stricter types:
- Document each prop with JSDoc
- Consider replacing
Record<string, unknown>with more specific typesexport interface CustomComponentProps { + /** Executes a custom widget event with the given name and context */ execute: (eventName: string, contextObj: Record<string, unknown>) => void; + /** Updates the widget's model with new data */ update: (data: Record<string, unknown>) => void; // ... add docs for other props }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 98-98: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 107-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 120-120: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (3)
4-14: Consider addressing TypeScript ignores and raw script loading
Multiple @ts-ignore comments and raw script loading could pose security risks. Consider:
- Adding proper types for raw-loader imports
- Implementing content security policies for loaded scripts
67-111: 🛠️ Refactor suggestion
Add error boundaries and improve switch case safety
The message handling logic needs two improvements:
- Add error boundaries to handle message processing failures
- Wrap case blocks in curly braces to prevent scoping issues
switch (message.type) {
case EVENTS.CUSTOM_WIDGET_READY: {
setIsIframeReady(true);
// ... rest of the code ...
break;
}
case EVENTS.CUSTOM_WIDGET_UPDATE_HEIGHT: {
const height = message.data.height;
// ... rest of the code ...
break;
}
// ... other cases ...
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 107-108: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
195-208: Review iframe sandbox security configuration
The disableIframeWidgetSandbox flag could expose security vulnerabilities if enabled. Ensure:
- The flag is only enabled in development
- Sandbox permissions are minimal for production use
✅ Verification successful
Sandbox configuration is properly controlled by environment settings
The iframe sandbox configuration is secure by default:
- Sandbox is enabled by default (disableIframeWidgetSandbox defaults to false)
- Can only be disabled via explicit environment variable APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX
- Uses appropriate sandbox permissions for widget functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other iframe security configurations
rg "disableIframeWidgetSandbox" --type ts
Length of output: 1192
Script:
#!/bin/bash
# Check the environment configuration setup
rg "DISABLE_IFRAME_WIDGET_SANDBOX" --type ts -A 2
Length of output: 377
Script:
#!/bin/bash
# Check for any security-related comments or documentation
rg "disableIframeWidgetSandbox" -g "*.md" -g "*.ts" -g "*.tsx" -B 2 -A 2
Length of output: 4651
Script:
#!/bin/bash
# Check the actual config implementation
cat app/client/src/ce/configs/index.ts
Length of output: 9776
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12117306473. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
38-39: Remove unnecessary eslint disable commentThe height state variable is used in the
CUSTOM_WIDGET_UPDATE_HEIGHThandler. Remove the@typescript-eslint/no-unused-varscomment.
126-166: Simplify conditional checks with optional chainingMultiple instances of
iframe.current && iframe.current.contentWindowcan be simplified using optional chaining.- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 127-127: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 157-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 105-105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 114-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 127-127: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 157-157: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (3)
104-112: Wrap case block in curly braces
The height variable declaration in the switch case should be wrapped in a block to prevent scope leakage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
194-222: LGTM! Proper implementation of iframe rendering
The implementation includes appropriate loading states, sandbox restrictions, and container handling.
29-29:
Review security implications of disabled sandbox
The disableIframeWidgetSandbox configuration could expose security vulnerabilities if enabled. Ensure this is only used in controlled environments.
✅ Verification successful
Let me search for any security documentation or configuration details about this feature.
Let me try a different approach to find documentation and security-related files.
Based on the search results, I can now provide a final response about the security implications of the iframe sandbox configuration:
Sandbox configuration is properly implemented with secure defaults
The codebase shows that disableIframeWidgetSandbox is used with secure defaults:
- When disabled (default), it applies strict sandbox permissions: "allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts"
- It's configurable via environment variable with false as default
- It's consistently implemented across iframe widgets
- The sandbox is only disabled in controlled environments through explicit configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of disableIframeWidgetSandbox in configs and security documentation
rg -A 5 "disableIframeWidgetSandbox"
Length of output: 6198
Script:
#!/bin/bash
# Search for documentation about this feature in markdown files and comments
rg -i "disableiframewidgetsandbox|iframe.+sandbox" "*.md" "*.mdx"
# Also check for any environment configuration documentation
rg -i "APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX" "*.md" "*.mdx" "*.env*"
Length of output: 367
Script:
#!/bin/bash
# Find documentation files first
fd -e md -e mdx
# Then search in these files along with any security-related paths
rg -i "disableiframewidgetsandbox|iframe.+sandbox|APPSMITH_DISABLE_IFRAME_WIDGET_SANDBOX" -g "!node_modules"
# Look for environment configuration files
fd -g ".env*"
Length of output: 73054
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
Outdated
Show resolved
Hide resolved
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| width: (props as any).width, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| height: (props as any).height, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type assertions with proper types
Using as any bypasses TypeScript's type checking. Define proper interfaces for the width and height properties.
- width: (props as any).width,
- height: (props as any).height,
+ width: props.width as number,
+ height: props.height as number,📝 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.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| width: (props as any).width, | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| height: (props as any).height, | |
| }, | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| width: props.width as number, | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| height: props.height as number, | |
| }, |
| export interface CustomComponentProps { | ||
| execute: (eventName: string, contextObj: Record<string, unknown>) => void; | ||
| update: (data: Record<string, unknown>) => void; | ||
| model: Record<string, unknown>; | ||
| srcDoc: { | ||
| html: string; | ||
| js: string; | ||
| css: string; | ||
| }; | ||
| onLoadingStateChange?: (state: string) => void; | ||
| needsOverlay?: boolean; | ||
| onConsole?: (type: string, message: string) => void; | ||
| renderMode: "EDITOR" | "DEPLOYED" | "BUILDER"; | ||
| theme: AppThemeProperties; | ||
| widgetId: string; | ||
| elevatedBackground: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing width and height properties to interface
The interface is missing width and height properties that are used in the component.
export interface CustomComponentProps {
+ width: number;
+ height: number;
execute: (eventName: string, contextObj: Record<string, unknown>) => void;
// ... rest of the propertiesCommittable suggestion skipped: line range outside the PR's diff.
| const srcDoc = ` | ||
| <html> | ||
| <head> | ||
| <style>${css}</style> | ||
| </head> | ||
| <body> | ||
| <script type="text/javascript">${ | ||
| props.onConsole ? appsmithConsole : "" | ||
| }</script> | ||
| <script type="module"> | ||
| ${script} | ||
| main(); | ||
| </script> | ||
| ${props.srcDoc.html} | ||
| <script type="module"> | ||
| ${props.srcDoc.js} | ||
| </script> | ||
| <style>${props.srcDoc.css}</style> | ||
| </body> | ||
| </html> | ||
| `; |
There was a problem hiding this comment.
💡 Codebase verification
Add HTML content sanitization to prevent XSS attacks
The custom widget directly injects HTML, JS, and CSS from props.srcDoc into an iframe without sanitization. While iframes provide some isolation, unsanitized content injection can still lead to XSS vulnerabilities.
- Add DOMPurify or a similar HTML sanitization library to the project dependencies
- Sanitize HTML content before injection:
const sanitizedHtml = DOMPurify.sanitize(props.srcDoc.html);
const sanitizedCss = DOMPurify.sanitize(props.srcDoc.css);
const sanitizedJs = DOMPurify.sanitize(props.srcDoc.js);🔗 Analysis chain
Verify content sanitization
The srcDoc template injects HTML, JS, and CSS directly. Ensure that props.srcDoc content is properly sanitized to prevent XSS attacks.
Let me search for any sanitization related to HTML/JS content in the custom widget component.
Let me check if there are any security-related utilities or sanitization helpers being used in this component.
Let me check the evaluation worker to see if there's any content sanitization being applied.
Let me check if there are any security-related utilities being used in the component's dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sanitization utilities in the codebase
rg -A 5 "sanitize|purify|DOMPurify"
Length of output: 70484
Script:
#!/bin/bash
# Search for sanitization in the custom widget component and related files
rg -A 5 "sanitize|purify|DOMPurify" "app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget"
Length of output: 94
Script:
#!/bin/bash
# Look for imports and security-related utilities in the component
rg -A 5 "import.*(?:sanitize|security|xss|html|DOMPurify)" "app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget"
# Also check for any security-related utilities in the codebase that might be used
rg -l "export.*(?:sanitizeHTML|sanitizeScript|sanitizeCSS)" "app/client/src"
Length of output: 240
Script:
#!/bin/bash
# Check the evaluation worker for sanitization functions
cat app/client/src/workers/Evaluation/evaluate.ts
Length of output: 14964
Script:
#!/bin/bash
# Check for security-related imports in the component's dependencies
rg -l "import.*(?:DOMPurify|xss-filters|sanitize-html)" app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget
# Also check if there's any HTML sanitization happening in the component
rg -A 5 "dangerouslySetInnerHTML|srcDoc" app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget
Length of output: 6396
|
Deploy-Preview-URL: https://ce-37878.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12154881651. |
|
Deploy-Preview-URL: https://ce-37878.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12155498247. |
|
Deploy-Preview-URL: https://ce-37878.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12159990812. |
|
Deploy-Preview-URL: https://ce-37878.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (14)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css (2)
1-6: Consider adding a fallback value for --component-heightThe CSS variable usage is clean, but adding a fallback value for --component-height would improve robustness.
.container { - height: var(--component-height, 100%); + height: var(--component-height, 300px); max-height: var(--max-height, unset); border-radius: inherit; overflow: hidden; }
15-17: Add documentation for data-size attributePlease document the purpose and possible values of the data-size attribute to improve maintainability.
+/* data-size="fit-page" ensures the iframe content fits within its container without scrolling */ .iframe[data-size="fit-page"] { max-height: 100%; }app/client/packages/design-system/theming/src/utils/cssRule.ts (1)
Line range hint
27-33: Remove @ts-expect-error by improving type definitionsThe presence of type assertion bypasses suggests underlying type definition issues. Consider improving the Theme type definitions instead.
- objectKeys(token).forEach((key) => { - //@ts-expect-error: type mismatch - styles += `--${kebabCase(token[key].type)}-${kebabCase(key)}: ${ - //@ts-expect-error: type mismatch - token[key].value - };`; + type TokenValue = { type: string; value: string }; + objectKeys(token).forEach((key) => { + const tokenValue = token[key] as TokenValue; + if (!tokenValue?.type || !tokenValue?.value) return; + styles += `--${kebabCase(tokenValue.type)}-${kebabCase(key)}: ${tokenValue.value};`; });app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js (3)
93-106: Enhance error handling for non-cloneable dataThe current error handling simply rethrows the error without providing context about the failure.
Consider enhancing the error handling:
- } catch (e) { - throw e; + } catch (e) { + throw new Error(`Failed to post message: ${e.message}. Data might not be cloneable.`); }🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
273-276: Consider using immutable model updatesDirect model mutation could lead to inconsistencies. Consider using a more immutable approach.
Consider this pattern:
- appsmith.model = Object.assign( - Object.assign({}, appsmith.model), - obj, - ); + appsmith.model = { + ...appsmith.model, + ...obj + };
322-322: Add validation for provider parameterThe provider parameter should be validated to ensure it's a valid value.
Consider adding validation:
export const generateAppsmithCssVariables = (provider) => (source) => { + if (!['model', 'ui'].includes(provider)) { + throw new Error('Provider must be either "model" or "ui"'); + } let cssTokens = document.getElementById(`appsmith-${provider}-css-tokens`);app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts (1)
1-6: Consider moving documentation URL to widget configuration.The documentation URL should be injected through widget configuration rather than directly referenced in the template. This would make the template more maintainable and allow for environment-specific URLs.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx (1)
Line range hint
51-92: Refactor to eliminate duplicate dimension calculationsConsolidate the dimension calculation logic to improve maintainability.
Here's a possible refactoring:
+ const updateDimensions = () => { + if (containerRef.current) { + setDimensions({ + width: containerRef.current.clientWidth + 8, + height: + window.innerHeight - + containerRef.current.getBoundingClientRect().top - + 31, + }); + } + }; useEffect(() => { - if (containerRef.current) { - setDimensions({ - width: containerRef.current?.clientWidth + 8, - height: - window.innerHeight - - containerRef.current.getBoundingClientRect().top - - 31, - }); - } + updateDimensions(); const handleResize = () => { - if (containerRef.current) { - setDimensions({ - width: containerRef.current?.clientWidth + 8, - height: - window.innerHeight - - containerRef.current.getBoundingClientRect().top - - 31, - }); - } + updateDimensions(); }; window.addEventListener("resize", handleResize); return () => { window.removeEventListener("resize", handleResize); }; }, []); useEffect(() => { - if (containerRef.current) { - setDimensions({ - width: containerRef.current.clientWidth + 8, - height: - window.innerHeight - - containerRef.current.getBoundingClientRect().top - - 31, - }); - } + updateDimensions(); }, [width, containerRef.current?.clientWidth]);app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
1-6: Consider grouping related importsThe imports are well-organized but could be grouped better for maintainability:
- External libraries (lodash)
- Internal utilities (@AppSmith)
- Constants
-import { objectKeys } from "@appsmith/utils"; -import startCase from "lodash/startCase"; -import capitalize from "lodash/capitalize"; -import { ValidationTypes } from "constants/WidgetValidation"; -import { COMPONENT_SIZE } from "../../constants"; +// External libraries +import startCase from "lodash/startCase"; +import capitalize from "lodash/capitalize"; + +// Internal utilities +import { objectKeys } from "@appsmith/utils"; +import { ValidationTypes } from "constants/WidgetValidation"; +import { COMPONENT_SIZE } from "../../constants";app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (1)
15-16: Consider extracting magic numbers as constantsThe
rowsandcolumnsvalues should be defined as named constants to improve maintainability and document their purpose.+const DEFAULT_WIDGET_DIMENSIONS = { + ROWS: 30, + COLUMNS: 23, +} as const; + export const defaultsConfig = { widgetName: "Custom", - rows: 30, - columns: 23, + rows: DEFAULT_WIDGET_DIMENSIONS.ROWS, + columns: DEFAULT_WIDGET_DIMENSIONS.COLUMNS,app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/react.ts (1)
44-46: Avoid using !important in CSS rulesUsing !important flags makes styles hard to override and maintain. Consider restructuring CSS selectors for better specificity.
Also applies to: 48-51
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vue.ts (1)
66-70: Simplify button stylesConsider using CSS custom properties for button styles to improve maintainability.
.button-container button#next { - background: var(--appsmith-theme-color-bg-accent) !important; - color: #fff; - border:1px solid var(--appsmith-theme-color-bg-accent) !important; + background: var(--button-primary-bg, var(--appsmith-theme-color-bg-accent)); + color: var(--button-primary-color, #fff); + border: 1px solid var(--button-primary-border, var(--appsmith-theme-color-bg-accent)); }Also applies to: 72-77
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
5-15: Consider using proper TypeScript configurations for raw-loader imports.Multiple
@ts-ignorecomments indicate potential type safety issues. Consider:
- Creating type declarations for raw-loader imports
- Configuring webpack/TypeScript to handle these imports properly
198-213: Add missing width and height properties to interface.The interface is missing width and height properties that might be needed for proper component sizing.
export interface CustomComponentProps { + width?: number; + height?: number; execute: (eventName: string, contextObj: Record<string, unknown>) => void; // ... rest of the properties
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
app/client/packages/design-system/theming/src/utils/cssRule.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/useCustomWidgetHeight.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/constants.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/helpers/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/index.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/react.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vue.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/index.tsx(2 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx(4 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/styles.module.css(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/index.tsx(2 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/styles.module.css(1 hunks)app/client/src/utils/CustomWidgetBuilderService.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/pages/Editor/CustomWidgetBuilder/Preview/styles.module.css
- app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts
🧰 Additional context used
📓 Learnings (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
Learnt from: riodeuno
PR: appsmithorg/appsmith#30351
File: app/client/src/widgets/wds/WDSModalWidget/config/propertyPaneConfig/styleConfig.ts:1-3
Timestamp: 2024-11-12T08:11:25.417Z
Learning: The `WDSModalWidget` currently has no style configurations, and they will be added in the future if needed.
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 23-23: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 101-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 109-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 122-122: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 134-134: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
[error] 66-66: 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)
🔇 Additional comments (16)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css (2)
8-13: LGTM!
Clean and efficient iframe styling implementation.
28-48: Verify spacing calculations in different viewport sizes
The spacing calculations look correct, but please verify:
- Widget behavior in different viewport sizes
- Interaction with parent containers
- Impact on responsive layouts
✅ Verification successful
Spacing calculations are correctly defined and consistently used
The CSS variables are well-defined in the design system:
--outer-spacing-3: 8.9px--outer-spacing-4: 13.2px
The calculations in the widget styles are correct:
- Single elevation:
height - (8.9px * 2) - Double elevation:
height - (8.9px * 4)
These spacing values are consistently used across other components (Modal, Calendar, Table) for padding and margins.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related spacing variable definitions
rg --type css "outer-spacing-[34]" -A 2
Length of output: 5437
app/client/packages/design-system/theming/src/utils/cssRule.ts (1)
4-5: Verify type-safety benefits of objectKeys utility
Let's confirm if this custom utility provides additional type safety benefits over Object.keys().
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about the objectKeys utility:
The objectKeys utility is correctly used for type-safe key iteration
The utility provides proper type inference by returning Array<keyof T> instead of string[], making it a safer choice over Object.keys(). This is further enforced by an ESLint rule that warns against using Object.keys directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of objectKeys utility
rg -A 5 "export.*objectKeys"
Length of output: 939
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js (3)
1-13: LGTM! Well-structured event constants.
The event constants are well-defined with clear naming conventions and cover all necessary communication scenarios.
23-23: Replace 'instanceof Array' with 'Array.isArray()'
For more reliable array type checking across different execution contexts.
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
182-207: LGTM! Secure theme update implementation.
The theme update handler properly manages style elements and safely injects CSS tokens.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx (1)
66-66: 🛠️ Refactor suggestion
Use 'Object.hasOwn()' instead of 'hasOwnProperty'
Using hasOwnProperty directly on objects can be unreliable. Use Object.hasOwn() for safer property checks.
Apply this diff to fix the issue:
- if (this.props.hasOwnProperty(eventName)) {
+ if (Object.hasOwn(this.props, eventName)) {🧰 Tools
🪛 Biome (1.9.4)
[error] 66-66: 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)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/constants.ts (1)
1-14: LGTM
Constants are well-defined and correctly exported.
app/client/src/pages/Editor/CustomWidgetBuilder/styles.module.css (1)
61-65: Consider browser compatibility for :has() selector
The CSS :has() selector is relatively new and might not be supported in all browsers. Consider adding a fallback mechanism or checking browser support requirements.
Let's check the browser requirements in your project:
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
7-36: LGTM! Well-structured property pane configuration
The configuration is well-organized with proper validation and type safety.
app/client/src/pages/Editor/CustomWidgetBuilder/index.tsx (2)
78-78: LGTM!
The addition of the styles.preview className to the Preview component is appropriate for styling purposes.
90-91: LGTM!
The data-resizing attribute is correctly implemented for handling resize state styling.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/index.tsx (3)
16-17: LGTM!
The separation of templates based on layout types improves code organization.
24-25: LGTM!
The Redux imports are appropriate for accessing the layout state.
82-83: LGTM!
The conditional template selection based on layout type is well implemented.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
100-108: 🛠️ Refactor suggestion
Wrap variable declaration in switch case block.
The height variable declaration in the switch case should be wrapped in a block to prevent scope issues.
case EVENTS.CUSTOM_WIDGET_UPDATE_HEIGHT:
+ {
const height = message.data.height;
if (props.renderMode !== "BUILDER" && height) {
iframe.current?.style.setProperty("height", `${height}px`);
}
break;
+ }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
Show resolved
Hide resolved
...Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts
Show resolved
Hide resolved
...Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts
Show resolved
Hide resolved
...Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts
Show resolved
Hide resolved
...ges/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/react.ts
Show resolved
Hide resolved
...ges/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/react.ts
Show resolved
Hide resolved
...pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vue.ts
Show resolved
Hide resolved
| new Vue({ | ||
| el: "#app", | ||
| data() { | ||
| return { | ||
| currentIndex: 0, | ||
| tips: appsmith.model.tips, | ||
| }; | ||
| }, | ||
| methods: { | ||
| next() { | ||
| this.currentIndex = (this.currentIndex + 1) % this.tips.length; | ||
| }, | ||
| reset() { | ||
| this.currentIndex = 0; | ||
| appsmith.triggerEvent("onReset"); | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for appsmith.model access
Add proper error handling and type checking for the appsmith.model object.
data() {
return {
currentIndex: 0,
- tips: appsmith.model.tips,
+ tips: appsmith?.model?.tips || [],
};
},
+created() {
+ if (!appsmith?.model?.tips?.length) {
+ console.error('Tips data not available');
+ }
+},📝 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.
| new Vue({ | |
| el: "#app", | |
| data() { | |
| return { | |
| currentIndex: 0, | |
| tips: appsmith.model.tips, | |
| }; | |
| }, | |
| methods: { | |
| next() { | |
| this.currentIndex = (this.currentIndex + 1) % this.tips.length; | |
| }, | |
| reset() { | |
| this.currentIndex = 0; | |
| appsmith.triggerEvent("onReset"); | |
| }, | |
| }, | |
| }); | |
| new Vue({ | |
| el: "#app", | |
| data() { | |
| return { | |
| currentIndex: 0, | |
| tips: appsmith?.model?.tips || [], | |
| }; | |
| }, | |
| created() { | |
| if (!appsmith?.model?.tips?.length) { | |
| console.error('Tips data not available'); | |
| } | |
| }, | |
| methods: { | |
| next() { | |
| this.currentIndex = (this.currentIndex + 1) % this.tips.length; | |
| }, | |
| reset() { | |
| this.currentIndex = 0; | |
| appsmith.triggerEvent("onReset"); | |
| }, | |
| }, | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts (1)
325-327: Consider using proper typing for mock clearInstead of type casting with
any, consider defining proper types for the mock implementation.- (window.parent.postMessage as any).mockClear(); + const mockedPostMessage = window.parent.postMessage as jest.MockedFunction<typeof window.parent.postMessage>; + mockedPostMessage.mockClear();app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js (1)
93-106: Simplify error handling in postMessageThe try-catch block that only rethrows the error is unnecessary and can be simplified.
- postMessage: (type, data) => { - try { - // Try block to catch non clonealbe data error while postmessaging - // throw error if the data is not cloneable - postMessageQueue.push({ - type, - data, - }); - scheduleMicrotaskToflushPostMessageQueue(); - } catch (e) { - throw e; - } - }, + postMessage: (type, data) => { + // Non-cloneable data will throw error during postMessage + postMessageQueue.push({ + type, + data, + }); + scheduleMicrotaskToflushPostMessageQueue(); + },🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
[error] 104-104: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 23-23: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
🔇 Additional comments (9)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts (4)
14-19: Define proper TypeScript interfaces for window properties
The existing TODO comments indicate the need for proper typing. Consider defining proper interfaces for the appsmith and triggerEvent window properties as suggested in the previous review.
23-103: LGTM! Comprehensive test coverage for createChannelToParent
The tests thoroughly cover both the message handling and postMessage functionality, including proper error cases and asynchronous behavior.
105-124: LGTM! Well-structured test for CSS variable generation
The test effectively verifies the CSS variable generation functionality with proper assertions.
126-353: LGTM! Comprehensive test coverage for CustomWidgetScript API
The tests thoroughly cover all API functions, state management, and event handling.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js (5)
23-23: Use Array.isArray() instead of instanceof Array
Replace instanceof Array with Array.isArray() for reliable array type checking across different execution contexts.
🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Use Array.isArray() instead of instanceof Array.
instanceof Array returns false for array-like objects and arrays from other execution contexts.
Unsafe fix: Use Array.isArray() instead.
(lint/suspicious/useIsArray)
131-137: Consider cleanup for ResizeObserver
The ResizeObserver should be disconnected when no longer needed to prevent memory leaks.
160-211: LGTM! Well-implemented event handlers
The event handlers properly manage state transitions and subscriber notifications, with clean DOM manipulation for theme updates.
213-314: LGTM! Robust appsmith global object implementation
The implementation includes proper error handling, clean subscriber management, and unsubscribe functionality.
322-347: LGTM! Clean CSS variable generation implementation
The function efficiently generates and updates CSS variables with proper DOM manipulation.
|
|
||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| window.addEventListener = (type: string, handler: any) => { |
There was a problem hiding this comment.
Could you fix types in this file? It is possible to do this as separate task.
There was a problem hiding this comment.
Cool. I am anyway working on a separate branch where I am refactor all the code of custom to make it a little more readable with comments that makes sense.
| import kebabCase from "lodash/kebabCase"; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore |
There was a problem hiding this comment.
(nitpick) Better to use @ts-expect-error
| size?: keyof typeof COMPONENT_SIZE; | ||
| } | ||
|
|
||
| export default CustomComponent; |
|
|
||
| export const defaultsConfig = { | ||
| widgetName: "Custom", | ||
| rows: 30, |
There was a problem hiding this comment.
rows and columns not needed here.
|
|
||
| import type { CustomWidgetProps } from "../../types"; | ||
|
|
||
| const StyledLink = styled(Link)` |
There was a problem hiding this comment.
Why do we need these styles?
There was a problem hiding this comment.
To style the "Read more" here. ADS link has no size variant 🤔
It looks like this with above styles.:
There was a problem hiding this comment.
I've also seen inline styles, let's describe the link in one place?
| reactDom.render(<App />, document.getElementById("root")); | ||
| });`, | ||
| }, | ||
| srcDoc: { |
There was a problem hiding this comment.
@jsartisan Do you understand how this works? What is the difference between the code that is written here and the code that is above? How did we get this string?
There was a problem hiding this comment.
The iframe renders the compiled code, and the compilation happen only when we change code in the CustomWidgetBuilder tab, and compiled code is then saved to srcDoc.
But for new widgets on drop, compilation flow does not happen. This is why Balaji maintained two code here, compiled and uncompiled. Uncompiled code is used to show in code of the editor, compiled is what is sent to iframe.
This seems all wrong IMO. We should just maintain uncompiled code and compile it when rendering the iframe.
But that would require some effort. so skipping that for now.
There was a problem hiding this comment.
Please create a task for this.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
Outdated
Show resolved
Hide resolved
| appsmith.triggerEvent("onReset"); | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
What is the difference with defaultApp.ts file? Why is the code duplicated here?
There was a problem hiding this comment.
🤷 Not sure. I think it is because the templates are stored in CustomWidgetBuilder and balaji wanted to avoid the import from there :|
But I agree that we should import all this in default App instead of repeating in the defaultApp.
| const customComponent = () => { | ||
| if (isAnvilLayout) { | ||
| return ( | ||
| <ThemeProvider className={styles.themeProvider} theme={anvilTheme}> |
There was a problem hiding this comment.
Why do we need ThemeProvider here if we don't have access to CSS variables inside the iframe anyway?
There was a problem hiding this comment.
CustomComponent grabs from the ThemeProvider and attaches the cssTokens to the src. This way iframe has our cssTokens.
| @@ -0,0 +1,4 @@ | |||
| .themeProvider { | |||
There was a problem hiding this comment.
Preview renders the CustomComponent. CustomComponent is responsible for grabbing the theme tokens and generate CSS variable and attach to the srcDoc which is sent to Iframe.
All this preview code is happening outside of our main application. So it does not have ThemeProvider in the tree. So need to wrap custom component in ThemeProvider so that theme tokens are available.
|
@jsartisan Please add this widget to the exceptions for interaction as we did before. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
app/client/src/layoutSystems/anvil/editor/styles.module.css (1)
Line range hint
6-9: Consider tracking this temporary solutionThe comment indicates this is a temporary solution pending full inline editing feature implementation.
Would you like me to create a tracking issue to ensure this temporary solution is revisited when the full-fledged inline editing feature is implemented?
Also applies to: 11-11
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx (2)
33-42: Consider memoizing theme props.The
wdsThemePropsobject is recreated on every render. Consider using useMemo to optimize performance.- const wdsThemeProps = { + const wdsThemeProps = React.useMemo(() => ({ borderRadius: themeSetting.borderRadius, seedColor: themeSetting.accentColor, colorMode: themeSetting.colorMode.toLowerCase(), userSizing: themeSetting.sizing, userDensity: themeSetting.density, - } as Parameters<typeof useTheme>[0]; + }), [themeSetting]) as Parameters<typeof useTheme>[0];
93-123: Add type safety to event handlers.The
contextObjectparameter is typed asunknown. Consider defining a proper type interface for better type safety and documentation.+ interface EventContext { + // Add your event context properties here + [key: string]: unknown; + } - const execute = (name: string, contextObject: unknown) => { + const execute = (name: string, contextObject: EventContext) => {app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (2)
17-17: Consider making the reset message configurableThe hardcoded alert message reduces reusability. Consider making it a configurable property.
-onResetClick: "{{showAlert('Successfully reset!!', '');}}", +onResetClick: "{{showAlert(this.resetMessage || 'Successfully reset!!', '');}}",
17-17: Consider using a more enterprise-friendly notification systemUsing
alert()might not align with enterprise UI patterns. Consider using your application's toast or notification system instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/src/layoutSystems/anvil/editor/styles.module.css(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 98-98: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 106-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
[error] 64-64: 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)
🔇 Additional comments (9)
app/client/src/layoutSystems/anvil/editor/styles.module.css (1)
11-11: Clean implementation using :is() selector
The selector modification elegantly extends widget interaction support to include Custom widgets while maintaining existing functionality.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx (3)
2-3: LGTM: Component signature and imports are well structured.
The addition of layout-specific components and the optional className prop provides good flexibility for different layout systems.
Also applies to: 17-23
Line range hint 125-163: Well-structured conditional rendering implementation.
The component properly handles different layout types and maintains consistent theming. The previous concern about ThemeProvider has been addressed through proper CSS token handling.
165-170: LGTM: Clean and maintainable return statement.
The implementation properly uses the className prop and ref handling.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx (2)
63-81: 🛠️ Refactor suggestion
Use Object.hasOwn() for safer property checks
Replace direct hasOwnProperty calls with Object.hasOwn() for better safety.
-if (this.props.hasOwnProperty(eventName)) {
+if (Object.hasOwn(this.props, eventName)) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 64-64: 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)
83-92:
Add null check for model property
The model spread operation could fail if model is undefined.
update = (data: Record<string, unknown>) => {
this.props.updateWidgetMetaProperty("model", {
- ...this.props.model,
+ ...(this.props.model || {}),
...data,
});Likely invalid or redundant comment.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
97-105: 🛠️ Refactor suggestion
Wrap switch case declarations in blocks
Variables declared in switch cases should be wrapped in blocks to prevent scoping issues.
case EVENTS.CUSTOM_WIDGET_UPDATE_HEIGHT: {
const height = message.data.height;
if (props.renderMode !== "BUILDER" && height) {
iframe.current?.style.setProperty("height", `${height}px`);
}
break;
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (2)
1-12: LGTM! Clean import organization.
The imports are well-organized, properly typed, and follow a logical grouping of external dependencies followed by internal modules.
27-62: Extract blueprint operations to a separate file
The blueprint operations contain complex widget tree manipulation logic that would be better maintained in a separate utility file.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts
Show resolved
Hide resolved
I have done it but it causes difficulty in widget selection because of the iframe. |
How does it work in a fixed layout then? |
They had added an overlay that is shown until the widget is selected. I removed that code thinking we don't allow interaction in edit mode anyway. |
|
Hi Pawan, am I right that most of this code was copied from somewhere else and not written by you? |
|
@znamenskii-ilia Right. Just forked from fixed layout custom widget and added some changes as per anvil. |
/ok-to-test tags="@tag.Widget" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a custom widget with enhanced console logging and communication capabilities. - Added a configuration system for widget properties, including visibility, style settings, and autocomplete functionality. - Implemented a responsive design for the custom widget with dynamic loading events and error handling. - Expanded widget mapping to include the new custom widget type. - Added support for multiple code templates (React, Vue, Vanilla JS) for custom widget creation. - Introduced a custom hook for managing widget height based on component sizes and embedding status. - **Bug Fixes** - Resolved various issues related to event handling and message passing between the widget and parent context. - **Documentation** - Added comprehensive comments and structure to configuration files for better clarity and usability. - **Style** - Included a CSS reset stylesheet for consistent styling across browsers. - Introduced new CSS classes for improved widget styling. - Enhanced styling rules to manage pointer events during widget resizing. - **Tests** - Developed a test suite to ensure the reliability of the widget's functionality and event handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12194686716> > Commit: a757240 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12194686716&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget` > Spec: > <hr>Fri, 06 Dec 2024 08:28:58 UTC <!-- end of auto-generated comment: Cypress test results -->
/ok-to-test tags="@tag.Widget"
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style
Tests
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12194686716
Commit: a757240
Cypress dashboard.
Tags:
@tag.WidgetSpec:
Fri, 06 Dec 2024 08:28:58 UTC