chore: Refactor WDS custom widget #38038
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/helpers/index.ts (1)
13-13: Fix typographical error in the commentRemove the extra period at the end of the sentence.
Apply this diff:
- * @param isSandboxDisabled - boolean - The value comes from the admin settings.. + * @param isSandboxDisabled - boolean - The value comes from the admin settings.app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (1)
15-16: Document new configuration propertiesThe newly added
rowsandcolumnsproperties lack documentation explaining their purpose and constraints.+ /** Default number of rows for the widget layout */ rows: 30, + /** Default number of columns for the widget layout */ columns: 23,app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/types.ts (1)
45-53: Strengthen IframeMessage type safetyThe
dataandthemeproperties are typed asunknown, which could lead to runtime type errors.Consider creating specific types for these properties:
export interface IframeMessage { type: string; - data?: unknown; + data?: Record<string, unknown>; - theme?: unknown; + theme?: { + colors: Record<string, string>; + fonts: Record<string, string>; + // add other theme properties + }; key?: string; model?: Record<string, unknown>; success?: boolean; mode?: CustomWidgetComponentProps["renderMode"]; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/createHtmlTemplate.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js(0 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(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/helpers/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/services/IframeMessenger.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/index.tsx(7 hunks)
💤 Files with no reviewable changes (2)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/styles.module.css (2)
Line range hint 1-7: LGTM: Container width property addition is appropriate
The addition of width: 100% ensures consistent full-width behavior for the custom widget container.
Line range hint 21-57: Verify the max-height calculations with nested widgets
The max-height calculations look correct, but let's verify the behavior when widgets are nested with different elevation states.
✅ Verification successful
Max-height calculations are correctly implemented for nested widgets with elevation
The implementation correctly handles widget height calculations:
- The CSS selectors properly cascade for nested elevation states
- The height calculations in
styles.module.cssaccount for both single and multiple elevation scenarios - The
useCustomWidgetHeighthook properly manages height updates based on canvas size - The component implementation correctly applies elevation attributes and height styles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any related test files or visual regression tests
# Look for test files related to this component
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | grep -i "WDSCustomWidget"
# Look for visual regression tests
fd -e stories.tsx . | grep -i "WDSCustomWidget"
# Look for usage of elevation attributes
rg -l 'elevation=|data-elevation' --type tsx
Length of output: 201
Script:
#!/bin/bash
# Search for test files with correct extension for TypeScript/React
fd -e ts -e tsx . | grep -i "WDSCustomWidget"
# Look for stories with correct extension
fd -e tsx . | grep -i "stories"
# Look for usage of elevation attributes with correct file extensions
rg -l 'elevation=|data-elevation' --type ts --type tsx --type jsx --type js
# Search for any CSS files that might have elevation-related styles
fd -e css . | xargs rg -l 'elevation'
# Look for documentation or implementation details
rg -l 'WDSCustomWidget' --type ts --type tsx --type jsx --type js
Length of output: 10148
Script:
#!/bin/bash
# Check the implementation of useCustomWidgetHeight hook to understand height calculations
cat "app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/useCustomWidgetHeight.ts"
# Look at the component implementation to see how elevation is used
cat "app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx"
# Check if there are any related styles in the anvil system
cat "app/client/src/layoutSystems/anvil/common/styles.module.css"
Length of output: 12179
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/useCustomWidgetHeight.ts (1)
9-11: LGTM
The changes correctly derive isEmbed from the URL parameters.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/services/IframeMessenger.ts (1)
11-33: LGTM: Well-implemented message handling with security checks
The message handling implementation includes proper source validation and a robust acknowledgment system.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/types.ts (1)
25-43: LGTM: Well-documented component props
The CustomWidgetComponentProps interface is thoroughly documented with clear explanations for each property.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx (4)
11-11: LGTM! Import changes align with the component renaming.
Also applies to: 13-13
15-32: Excellent documentation addition!
The comprehensive documentation clearly explains the widget's architecture and communication mechanisms. This will be valuable for future maintenance.
Line range hint 79-97: LGTM! Method renames improve clarity.
The renaming of methods to onExecuteAction and onUpdateModel better reflects their event handling nature.
Also applies to: 99-107
131-138: LGTM! Component and prop updates are consistent.
The CustomWidgetComponent props align with the renamed methods.
✅ Verification successful
Props are correctly typed and match the component implementation
The interface CustomWidgetComponentProps in types.ts properly defines both onTriggerEvent and onUpdateModel props, and these are correctly used in the component implementation. The prop updates are consistent with the method renames.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify CustomWidgetComponent prop types
ast-grep --pattern 'interface CustomWidgetComponentProps {
$$$
onTriggerEvent: $_
onUpdateModel: $_
$$$
}'
Length of output: 116
Script:
#!/bin/bash
# Let's try to find the CustomWidgetComponent definition and its props
ast-grep --pattern 'type CustomWidgetComponentProps = {
$$$
}'
# Also search for interface definition
ast-grep --pattern 'interface CustomWidgetComponent {
$$$
}'
# Search for the component definition to see prop usage
ast-grep --pattern 'const CustomWidgetComponent = ({ $$ }: $_) => {
$$$
}'
# Backup search using ripgrep
rg -A 10 "CustomWidgetComponent.*Props"
Length of output: 3862
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/component/index.tsx
Show resolved
Hide resolved
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore | ||
| import css from "!!raw-loader!./reset.css"; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore | ||
| import script from "!!raw-loader!./customWidgetscript.js"; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| //@ts-ignore | ||
| import appsmithConsole from "!!raw-loader!./appsmithConsole.js"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using @ts-ignore directives
Instead of silencing TypeScript errors, consider properly typing the imports.
You can declare module definitions for these imports to resolve the TypeScript errors.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/services/IframeMessenger.ts
Outdated
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
25-25: Add proper typing for the messenger refThe messenger ref could benefit from explicit typing instead of allowing null.
- const messenger = useRef<IframeMessenger | null>(null); + const messenger = useRef<IframeMessenger>(null);
38-64: Optimize message handler setupConsider memoizing the message handlers to prevent unnecessary recreations on model changes.
+ const messageHandlers = useMemo(() => ({ - const messageHandlers = { [EVENTS.CUSTOM_WIDGET_READY]: handleIframeOnLoad, [EVENTS.CUSTOM_WIDGET_UPDATE_MODEL]: handleModelUpdate, [EVENTS.CUSTOM_WIDGET_TRIGGER_EVENT]: handleTriggerEvent, [EVENTS.CUSTOM_WIDGET_UPDATE_HEIGHT]: handleHeightUpdate, [EVENTS.CUSTOM_WIDGET_CONSOLE_EVENT]: handleConsoleEvent, - }; + }), []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.test.ts(0 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)
💤 Files with no reviewable changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
131-131: Simplify iframe checks using optional chaining
The condition checks can be simplified using optional chaining.
- if (iframe.current && iframe.current.contentWindow && isIframeReady) {
+ if (iframe.current?.contentWindow && isIframeReady) {Also applies to: 145-145
🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| <iframe | ||
| className={styles.iframe} | ||
| data-size={kebabCase(size)} | ||
| data-size={kebabCase(props.size)} | ||
| loading="lazy" | ||
| onLoad={() => { | ||
| setLoading(false); | ||
| }} | ||
| ref={iframe} | ||
| sandbox={ | ||
| disableIframeWidgetSandbox | ||
| ? undefined | ||
| : "allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts" | ||
| } | ||
| sandbox={getSandboxPermissions(disableIframeWidgetSandbox)} | ||
| srcDoc={srcDoc} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance iframe accessibility
Add title and aria-label attributes to improve accessibility.
<iframe
className={styles.iframe}
data-size={kebabCase(props.size)}
loading="lazy"
+ title="Custom Widget Content"
+ aria-label="Custom Widget Content Frame"
onLoad={() => {
setLoading(false);
}}
ref={iframe}
sandbox={getSandboxPermissions(disableIframeWidgetSandbox)}
srcDoc={srcDoc}
/>📝 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.
| <iframe | |
| className={styles.iframe} | |
| data-size={kebabCase(size)} | |
| data-size={kebabCase(props.size)} | |
| loading="lazy" | |
| onLoad={() => { | |
| setLoading(false); | |
| }} | |
| ref={iframe} | |
| sandbox={ | |
| disableIframeWidgetSandbox | |
| ? undefined | |
| : "allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts" | |
| } | |
| sandbox={getSandboxPermissions(disableIframeWidgetSandbox)} | |
| srcDoc={srcDoc} | |
| /> | |
| <iframe | |
| className={styles.iframe} | |
| data-size={kebabCase(props.size)} | |
| loading="lazy" | |
| title="Custom Widget Content" | |
| aria-label="Custom Widget Content Frame" | |
| onLoad={() => { | |
| setLoading(false); | |
| }} | |
| ref={iframe} | |
| sandbox={getSandboxPermissions(disableIframeWidgetSandbox)} | |
| srcDoc={srcDoc} | |
| /> |
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/IframeMessenger.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/createHtmlTemplate.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/createHtmlTemplate.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/IframeMessenger.ts (1)
15-21: Strong security implementation for message handling.
The source validation prevents unauthorized message injection from malicious scripts.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/index.tsx (1)
131-138: LGTM: Console handling and event prop updates.
The changes align with the new messaging system and provide better event handling.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (4)
38-64: Well-structured message handling setup.
Clean separation of concerns with individual handlers for different message types.
131-131: Simplify conditions using optional chaining.
The conditions can be simplified using optional chaining.
Apply these diffs:
- if (iframe.current && iframe.current.contentWindow && isIframeReady) {
+ if (iframe.current?.contentWindow && isIframeReady) {Also applies to: 145-145
🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
174-184: 🛠️ Refactor suggestion
Enhance iframe accessibility.
Add title and aria-label attributes to improve accessibility.
Apply this diff:
<iframe
className={styles.iframe}
data-size={kebabCase(props.size)}
loading="lazy"
+ title="Custom Widget Content"
+ aria-label="Custom Widget Content Frame"
onLoad={() => {
setLoading(false);
}}
ref={iframe}
sandbox={getSandboxPermissions(disableIframeWidgetSandbox)}
srcDoc={srcDoc}
/>82-84:
Add error handling for model updates.
The model update handler should include error handling.
Apply this diff:
const handleModelUpdate = (message: Record<string, unknown>) => {
+ try {
onUpdateModel(message.model as Record<string, unknown>);
+ } catch (error) {
+ console.error('Failed to update model:', error);
+ onConsole?.('error', ['Failed to update model', error]);
+ }
};Likely invalid or redundant comment.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/IframeMessenger.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vue.ts (1)
Line range hint
19-19: Consider updating Vue versionThe template uses Vue 2.1.6 which is significantly outdated. Consider upgrading to Vue 3.x for better performance and modern features.
-<script src="//cdnjs.cloudflare.com/ajax/libs/vue/2.1.6/vue.min.js"></script> +<script src="//cdnjs.cloudflare.com/ajax/libs/vue/3.3.4/vue.global.min.js"></script>app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/react.ts (1)
Line range hint
11-14: Consider unifying theme variable namesThe base React template uses different theme variable names compared to the anvil template:
--appsmith-theme-borderRadiusvs--appsmith-theme-border-radius-elevation-3--appsmith-theme-boxShadowvs no equivalent in anvil templateConsider standardizing these theme variables across all templates for better maintainability.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/helpers/index.ts (1)
Line range hint
8-15: Fix missing closing parenthesis in template literalThe CSS calc function is missing a closing parenthesis.
- if (isEmbed) return `calc(${canvasHeight}px`; + if (isEmbed) return `calc(${canvasHeight}px)`; - return `calc(${canvasHeight}px - (var(--outer-spacing-4) * 2)`; + return `calc(${canvasHeight}px - (var(--outer-spacing-4) * 2))`;app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
Line range hint
174-184: Enhance iframe accessibilityThe iframe lacks proper accessibility attributes.
<iframe className={styles.iframe} data-size={kebabCase(size)} loading="lazy" + title="Custom Widget Content" + aria-label="Custom Widget Content Frame" onLoad={() => { setLoading(false); }} ref={iframe} sandbox={getSandboxPermissions(disableIframeWidgetSandbox)} srcDoc={srcDoc} />
38-64: Consider extracting message handlers to a separate hookThe message handling setup could be moved to a custom hook for better organization and reusability.
function useIframeMessageHandler(iframe: RefObject<HTMLIFrameElement>, handlers: MessageHandlers) { useEffect(() => { if (!iframe.current) return; const messenger = new IframeMessenger(iframe.current); const handler = (event: MessageEvent) => { messenger.handleMessage(event, handlers); }; window.addEventListener("message", handler, false); return () => window.removeEventListener("message", handler, false); }, [iframe, handlers]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/createHtmlTemplate.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/helpers/index.ts(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.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/Templates/react.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/vanillaJs.ts(1 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/vue.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/createHtmlTemplate.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vanillaJs.ts (2)
Line range hint 1-143: Clean and well-structured implementation!
The template demonstrates good practices:
- Proper DOM manipulation and event handling
- Clean state management
- Good integration with Appsmith's widget framework
- Consistent use of theming variables
108-108: LGTM! Event name change improves clarity.
The event name change from "onReset" to "onResetClick" better describes the user interaction.
Let's verify this change is consistent across other template files:
✅ Verification successful
Event name "onResetClick" is consistently used across all template files
The verification confirms that all template files (React, Vue, and VanillaJS) consistently use "onResetClick" as the event name, both in the root templates directory and the anvil templates subdirectory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent event naming across template files
# Expect: All reset-related events use "onResetClick"
rg -g '*.{ts,js}' "onReset" app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/
Length of output: 983
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/react.ts (1)
66-66: Verify event handler registration for "onResetClick"
The event name change from "onReset" to "onResetClick" is more descriptive, but we should ensure this event name is properly registered in the widget's event handling system.
✅ Verification successful
Based on the search results, I can generate the final response as the verification is complete. The search shows that:
-
The
onResetClickevent is properly registered in the Custom Widget's configuration:- In
app/client/src/widgets/CustomWidget/widget/index.tsx:events: ["onResetClick"] - In
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts:events: ["onResetClick"]
- In
-
The event is consistently used across all template implementations:
- React template
- Vue template
- Vanilla JS template
-
The event name is not in the restricted list (
RESTRICTED_NAMESinCustomWidgetAddEventButtonControl.tsx).
Event name change is properly implemented and consistent
The change from "onReset" to "onResetClick" is valid and properly implemented across all template files. The new name is more descriptive and follows the widget's event naming conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for event handler registrations and usages
rg -g '!*.{css,html}' -A 2 -B 2 'onResetClick|onReset'
Length of output: 31390
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/anvilTemplates/vue.ts (1)
107-107: Event name change is consistent with React template
The event name update to "onResetClick" maintains consistency across framework templates.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/react.ts (1)
69-69: Event name change is consistent
The event name update to "onResetClick" maintains consistency across all templates.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/vanillaJs.ts (1)
111-111: LGTM! Consistent event naming across templates.
The event rename matches the Vue template implementation, maintaining consistency across widget variants.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (2)
131-131: Simplify conditions using optional chaining
The conditions can be simplified using optional chaining.
Also applies to: 145-145
🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
82-84:
Add error handling for model updates
The model update handler lacks error handling.
const handleModelUpdate = (message: Record<string, unknown>) => {
+ try {
onUpdateModel(message.model as Record<string, unknown>);
+ } catch (error) {
+ console.error('Failed to update model:', error);
+ onConsole?.('error', `Failed to update model: ${error}`);
+ }
};Likely invalid or redundant comment.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/CodeTemplates/Templates/vue.ts
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/widget/defaultApp.ts
Show resolved
Hide resolved
| export const getSandboxPermissions = (isSandboxDisabled: boolean) => { | ||
| if (isSandboxDisabled) return undefined; | ||
|
|
||
| return "allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more restrictive sandbox permissions
The current sandbox permissions might be too permissive. Consider restricting them based on actual widget requirements.
- return "allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-scripts";
+ const requiredPermissions = [
+ "allow-scripts", // Required for basic functionality
+ "allow-forms", // Only if form submission is needed
+ "allow-popups" // Only if popups are needed
+ ].filter(Boolean).join(" ");
+ return requiredPermissions;Committable suggestion skipped: line range outside the PR's diff.
jsartisan
changed the title
github-actions
bot
added
the
skip-changelog
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
131-131: Simplify conditions using optional chainingBoth conditions can be simplified using optional chaining.
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {Also applies to: 145-145
🧰 Tools
🪛 Biome (1.9.4)
[error] 131-131: 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(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (5)
18-26: LGTM! Clean component structure with proper state management
The component structure follows React best practices with clear prop destructuring and appropriate use of refs and state hooks.
28-37: LGTM! Efficient theme token generation
Good use of useMemo for CSS token generation with proper prefixing of theme variables.
Line range hint 175-184: Enhance iframe accessibility
Add title and aria-label attributes to improve accessibility.
<iframe
className={styles.iframe}
data-size={kebabCase(size)}
loading="lazy"
+ title="Custom Widget Content"
+ aria-label="Custom Widget Content Frame"
onLoad={() => {
setLoading(false);
}}
ref={iframe}
sandbox={getSandboxPermissions(disableIframeWidgetSandbox)}
srcDoc={srcDoc}
/>82-84:
Add error handling for model updates
Consider adding try-catch blocks for model updates to handle potential parsing errors.
const handleModelUpdate = (message: Record<string, unknown>) => {
+ try {
onUpdateModel(message.model as Record<string, unknown>);
+ } catch (error) {
+ console.error('Failed to update model:', error);
+ onConsole?.('error', 'Failed to update model');
+ }
};38-64: 🛠️ Refactor suggestion
Consider updating useEffect dependencies
The effect's dependencies array only includes model, but the effect uses other props and functions that should be included to prevent stale closures.
Consider updating the dependencies array:
- }, [model]);
+ }, [model, handleIframeOnLoad, handleModelUpdate, handleTriggerEvent, handleHeightUpdate, handleConsoleEvent]);
github-actions
bot
added
Anvil Pod
/ok-to-test tags="@tag.Widget" Fixes appsmithorg#38028 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced a new `IframeMessenger` class for improved communication between the parent window and iframe. - Added `onConsole`, `onTriggerEvent`, and `onUpdateModel` props to the `Preview` component for enhanced event handling. - Streamlined HTML template generation with the `createHtmlTemplate` function. - **Bug Fixes** - Removed unnecessary UI-related event handling from the widget, simplifying the communication structure. - Updated event names in template files from `"onReset"` to `"onResetClick"` for clarity. - **Refactor** - Renamed `CustomComponent` to `CustomWidgetComponent` for clarity. - Modularized message handling logic in the `CustomWidgetComponent`. - Refactored the `defaultApp.ts` to use dynamic template data instead of hardcoded values. - **Style** - Updated CSS for the `.container` class to enhance layout consistency. - **Tests** - Simplified test assertions in the `customWidgetscript` test suite. <!-- 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/12234751176> > Commit: eb55fbd > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12234751176&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget` > Spec: > <hr>Mon, 09 Dec 2024 12:47:13 UTC <!-- end of auto-generated comment: Cypress test results -->
/ok-to-test tags="@tag.Widget"
Fixes #38028
Summary by CodeRabbit
Summary by CodeRabbit
New Features
IframeMessengerclass for improved communication between the parent window and iframe.onConsole,onTriggerEvent, andonUpdateModelprops to thePreviewcomponent for enhanced event handling.createHtmlTemplatefunction.Bug Fixes
"onReset"to"onResetClick"for clarity.Refactor
CustomComponenttoCustomWidgetComponentfor clarity.CustomWidgetComponent.defaultApp.tsto use dynamic template data instead of hardcoded values.Style
.containerclass to enhance layout consistency.Tests
customWidgetscripttest suite.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12234751176
Commit: eb55fbd
Cypress dashboard.
Tags:
@tag.WidgetSpec:
Mon, 09 Dec 2024 12:47:13 UTC