chore: add custom widget to anvil #37875
Conversation
|
/build-deploy-preview skip-tests=true |
WalkthroughThe pull request introduces a new custom widget framework within the UI builder, comprising several JavaScript and TypeScript files. Key additions include a custom console implementation, configuration objects for widget properties, and a structured communication system between the widget and its parent window. The changes also enhance test coverage for the widget's functionality and introduce a CSS reset for consistent styling. Overall, the modifications aim to improve widget interaction, logging, and configuration management. 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
|
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12116317297. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (19)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/index.tsx (1)
111-112: Simplify code with optional chainingUse optional chaining to make property access safer and more concise.
Apply these diffs:
At lines 111-112:
- props.onConsole && - props.onConsole(message.data.type, message.data.args); + props.onConsole?.(message.data.type, message.data.args);At line 124:
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {At line 136:
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {At line 151:
- if (iframe.current && iframe.current.contentWindow && isIframeReady) { + if (iframe.current?.contentWindow && isIframeReady) {Also applies to: 124-124, 136-136, 151-151
🧰 Tools
🪛 Biome (1.9.4)
[error] 111-112: 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)
95-106: Remove unnecessary try-catch blockThe try-catch block that only rethrows the error is redundant and can be removed.
Apply this diff:
- 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: Add TypeScript type definitions for configuration object.The configuration object would benefit from explicit TypeScript interfaces to ensure type safety and better IDE support.
+interface SetterConfig { + path: string; + type: string; +} + +interface WidgetSetterConfig { + __setters: { + [key: string]: SetterConfig; + }; +} + -export const setterConfig = { +export const setterConfig: WidgetSetterConfig = { __setters: { setVisibility: { path: "isVisible", type: "boolean", }, }, };app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/anvilConfig.ts (1)
1-9: Extract magic values into constants and add type definitions.The configuration would be more maintainable with explicit types and constants for breakpoints and sizing values.
+const BREAKPOINTS = { + MOBILE: "180px", +} as const; + +const SIZING = { + FULL: "100%", + SIZING_30: "sizing-30", +} as const; + +interface WidgetSizeConfig { + base: string; + [breakpoint: string]: string; +} + +interface AnvilConfig { + isLargeWidget: boolean; + widgetSize: { + minWidth: WidgetSizeConfig; + }; +} + -export const anvilConfig = { +export const anvilConfig: AnvilConfig = { isLargeWidget: true, widgetSize: { minWidth: { - base: "100%", - "180px": "sizing-30", + base: SIZING.FULL, + [BREAKPOINTS.MOBILE]: SIZING.SIZING_30, }, }, };app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/metaConfig.ts (2)
14-14: Consider expanding search tags for better discoverability.The search tags array contains only "external". Consider adding more relevant terms to improve widget discoverability.
- searchTags: ["external"], + searchTags: ["external", "custom", "widget", "component"],
7-17: Add TypeScript interface for meta configuration.Define a clear contract for the meta configuration structure using TypeScript interfaces.
+interface WidgetMetaConfig { + name: string; + iconSVG: string; + thumbnailSVG: string; + needsMeta: boolean; + isCanvas: boolean; + tags: string[]; + searchTags: string[]; + isSearchWildcard: boolean; + hideCard: boolean; +} + -export const metaConfig = { +export const metaConfig: WidgetMetaConfig = { name: "Custom", iconSVG: IconSVG, thumbnailSVG: ThumbnailSVG, needsMeta: true, isCanvas: false, tags: [WIDGET_TAGS.DISPLAY], searchTags: ["external"], isSearchWildcard: true, hideCard: isAirgapped(), };app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/autocompleteConfig.ts (1)
7-13: Consider adding JSDoc documentation for the function.While the function implementation is clean and follows good practices, adding documentation would help explain the purpose and expected behavior of the configuration.
+/** + * Generates autocomplete configuration for custom widgets + * @param widget - The custom widget properties + * @param extraDefsToDefine - Optional additional type definitions + * @returns Configuration object with visibility and model definitions + */ export const autocompleteConfig = ( widget: CustomWidgetProps, extraDefsToDefine?: ExtraDef, ) => ({app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/constants.ts (1)
2-9: Consider using a typed constant instead of a string literal.While the JSON string works, using a typed constant would provide better type safety and IDE support.
-export const DEFAULT_MODEL = `{ - "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})" - ] -}`; +interface DefaultModel { + tips: string[]; +} + +export const DEFAULT_MODEL: DefaultModel = { + 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})" + ] +};app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/defaultsConfig.ts (1)
9-10: Consider making dimensions configurableHardcoded values for rows and columns might not be suitable for all use cases. Consider making these configurable or responsive.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/appsmithConsole.js (1)
32-34: Enhance error event handlingThe error event handler could be more comprehensive by including additional error details.
window.addEventListener("error", (event) => { - postMessage("error", [event.message]); + postMessage("error", [{ + message: event.message, + filename: event.filename, + lineno: event.lineno, + colno: event.colno + }]); });app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/styleConfig.ts (1)
3-24: LGTM with enhancement suggestionThe style configuration is well-structured with proper validation and documentation. Consider adding more enterprise-grade styling options like:
- Custom border radius
- Shadow depth
- Background opacity
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/widget/defaultApp.ts (1)
8-51: Optimize CSS structureConsider consolidating CSS rules to reduce duplication between uncompiled and compiled versions. Use a CSS preprocessor or CSS-in-JS solution for better maintainability.
Also applies to: 100-148
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts (4)
61-102: Improve async test reliabilityReplace
setTimeoutwith proper Jest async utilities for more reliable tests.- return new Promise((resolve) => { - setTimeout(() => { + return act(async () => { expect(window.parent.postMessage).toHaveBeenCalledWith( // ... ); - resolve(true); - }); - }); + });
105-124: Enhance CSS variable test coverageAdd test cases for:
- Empty source object
- Nested properties
- Invalid values
130-152: Extract common test setupMove the repeated event setup logic into a helper function to reduce duplication.
function setupMessageEventHandling() { const events = new Map(); window.addEventListener = (type: string, handler: Function) => { events.set(type, handler); }; window.triggerEvent = (type: string, event: unknown) => { events.get(type)(event); }; return events; }
384-386: Remove TODO comment and fix type castingReplace the TODO comment with proper type definition for
postMessage.(window.parent.postMessage as jest.Mock).mockClear();app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/propertyPaneConfig/contentConfig.tsx (3)
33-33: Type 'widget' as 'CustomWidgetProps' in 'dynamicDependencies'The parameter 'widget' in 'dynamicDependencies' relies on properties from 'CustomWidgetProps' like 'events'. Update its type for accurate typing.
97-98: Use 'CustomWidgetProps' instead of 'WidgetProps'In 'generateDynamicProperties', 'widgetProps' should be typed as 'CustomWidgetProps' since it accesses 'events' specific to custom widgets.
35-46: Consider extracting inline styles to styled componentsMove the inline styles in 'helperText' to styled components for consistency.
📜 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 (5)
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/types.ts (1)
1-5: Interface CustomWidgetProps looks good
The CustomWidgetProps interface is well-defined and properly extends WidgetProps.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/config/autocompleteConfig.ts (1)
1-5: LGTM! Clean imports with proper type definitions.
The imports are well-organized and properly typed.
app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/constants.ts (1)
1-5: LGTM! Well-defined event constants.
The load events are clearly defined and follow a logical progression.
app/client/src/modules/ui-builder/ui/wds/constants.ts (1)
64-64: LGTM! Widget constant follows naming convention
The addition maintains consistency with existing widget mapping pattern.
app/client/src/widgets/index.ts (1)
90-90: LGTM! Widget registration follows established pattern
The import and registration of WDSCustomWidget maintain consistency with existing widget organization.
Also applies to: 189-189
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| static getMetaPropertiesMap(): Record<string, any> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the TODO and avoid using any type
There's a TODO indicating this code needs fixing. Also, replace any with a more specific type for better type safety.
Apply this diff:
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- static getMetaPropertiesMap(): Record<string, any> {
+ static getMetaPropertiesMap(): Record<string, unknown> {Would you like assistance in addressing this TODO and specifying the appropriate type?
📝 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.
| // TODO: Fix this the next time the file is edited | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| static getMetaPropertiesMap(): Record<string, any> { | |
| static getMetaPropertiesMap(): Record<string, unknown> { |
| } | ||
|
|
||
| execute = (eventName: string, contextObj: Record<string, unknown>) => { | ||
| if (this.props.hasOwnProperty(eventName)) { |
There was a problem hiding this comment.
Use safer property check method
Accessing hasOwnProperty directly can lead to issues if the property is overridden. Use Object.prototype.hasOwnProperty.call(this.props, eventName) instead.
Apply this diff:
- if (this.props.hasOwnProperty(eventName)) {
+ if (Object.prototype.hasOwnProperty.call(this.props, eventName)) {Committable suggestion skipped: line range outside the PR's diff.
🧰 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)
| const height = message.data.height; | ||
|
|
||
| if (props.renderMode !== "BUILDER" && height) { | ||
| iframe.current?.style.setProperty("height", `${height}px`); | ||
| setHeight(height); | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
Wrap switch case code block to prevent variable scope leakage
Declare const height within a block to ensure it doesn't affect other cases.
Apply this diff:
case EVENTS.CUSTOM_WIDGET_UPDATE_HEIGHT:
+ {
const height = message.data.height;
if (props.renderMode !== "BUILDER" && height) {
iframe.current?.style.setProperty("height", `${height}px`);
setHeight(height);
}
break;
+ }📝 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.
| const height = message.data.height; | |
| if (props.renderMode !== "BUILDER" && height) { | |
| iframe.current?.style.setProperty("height", `${height}px`); | |
| setHeight(height); | |
| } | |
| break; | |
| { | |
| const height = message.data.height; | |
| if (props.renderMode !== "BUILDER" && height) { | |
| iframe.current?.style.setProperty("height", `${height}px`); | |
| setHeight(height); | |
| } | |
| break; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[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)
| function onMessage(type, fn) { | ||
| let eventHandlerList = onMessageMap.get(type); | ||
|
|
||
| if (eventHandlerList && eventHandlerList instanceof Array) { |
There was a problem hiding this comment.
Use Array.isArray() for array checking
Replace instanceof Array with Array.isArray() for accurate array detection.
Apply this diff:
- if (eventHandlerList && eventHandlerList instanceof Array) {
+ if (eventHandlerList && Array.isArray(eventHandlerList)) {📝 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.
| if (eventHandlerList && eventHandlerList instanceof Array) { | |
| if (eventHandlerList && Array.isArray(eventHandlerList)) { |
🧰 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)
| theme: { | ||
| primaryColor: "", | ||
| backgroundColor: "", | ||
| borderRadius: "", | ||
| boxShadow: "", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type definitions for theme properties.
Theme properties should have proper type definitions for better type safety.
+interface ThemeConfig {
+ primaryColor: string;
+ backgroundColor: string;
+ borderRadius: string;
+ boxShadow: string;
+}
+
theme: {
primaryColor: "",
backgroundColor: "",
borderRadius: "",
boxShadow: "",
},📝 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.
| theme: { | |
| primaryColor: "", | |
| backgroundColor: "", | |
| borderRadius: "", | |
| boxShadow: "", | |
| }, | |
| interface ThemeConfig { | |
| primaryColor: string; | |
| backgroundColor: string; | |
| borderRadius: string; | |
| boxShadow: string; | |
| } | |
| theme: { | |
| primaryColor: "", | |
| backgroundColor: "", | |
| borderRadius: "", | |
| boxShadow: "", | |
| }, |
| js: `import React from 'https://cdn.jsdelivr.net/npm/react@18.2.0/+esm' | ||
| import reactDom from 'https://cdn.jsdelivr.net/npm/react-dom@18.2.0/+esm' | ||
| import { Button, Card } from 'https://cdn.jsdelivr.net/npm/antd@5.11.1/+esm' | ||
| import Markdown from 'https://cdn.jsdelivr.net/npm/react-markdown@9.0.1/+esm'; |
There was a problem hiding this comment.
Consider security implications of CDN dependencies
External CDN dependencies without integrity checks could pose security risks. Consider:
- Hosting dependencies locally
- Adding integrity checks
- Implementing fallback mechanisms
Also applies to: 149-152
| const handleNext = () => { | ||
| setCurrentIndex((prevIndex) => (prevIndex + 1) % appsmith.model.tips.length); | ||
| }; |
There was a problem hiding this comment.
Add validation for appsmith.model.tips
Add null checks and validation for appsmith.model.tips to prevent runtime errors.
const handleNext = () => {
+ if (!Array.isArray(appsmith.model.tips) || !appsmith.model.tips.length) return;
setCurrentIndex((prevIndex) => (prevIndex + 1) % appsmith.model.tips.length);
};Also applies to: 156-158
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| appsmith: any; | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| triggerEvent: any; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any types with proper interfaces
Instead of using any and TODO comments, define proper interfaces for appsmith and triggerEvent.
interface AppsmithAPI {
model: Record<string, unknown>;
ui: { width: number; height: number };
theme: { color: string };
mode: string;
onReady: (handler: () => void) => void;
// Add other methods...
}
type TriggerEventFn = (type: string, event: MessageEvent) => void;| window.triggerEvent("message", { | ||
| source: window.parent, | ||
| data: { | ||
| type: EVENTS.CUSTOM_WIDGET_THEME_UPDATE, | ||
| theme: { | ||
| color: "#000", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(window.appsmith.theme).toEqual({ | ||
| color: "#000", | ||
| }); | ||
|
|
||
| expect(handler).toHaveBeenCalledWith( | ||
| { | ||
| color: "#000", | ||
| }, | ||
| { | ||
| color: "#fff", | ||
| }, | ||
| ); | ||
|
|
||
| handler.mockClear(); | ||
| unlisten(); | ||
|
|
||
| window.triggerEvent("message", { | ||
| source: window.parent, | ||
| data: { | ||
| type: EVENTS.CUSTOM_WIDGET_THEME_UPDATE, | ||
| theme: { | ||
| color: "#f0f", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(window.appsmith.theme).toEqual({ | ||
| color: "#f0f", | ||
| }); | ||
|
|
||
| expect(handler).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should check API functions - updateModel", async () => { | ||
| window.appsmith.updateModel({ | ||
| test: 4, | ||
| }); | ||
|
|
||
| return new Promise((resolve) => { | ||
| setTimeout(() => { | ||
| expect(window.parent.postMessage).toHaveBeenCalledWith( | ||
| { | ||
| type: EVENTS.CUSTOM_WIDGET_UPDATE_MODEL, | ||
| data: { | ||
| test: 4, | ||
| }, | ||
| key: expect.any(Number), | ||
| }, | ||
| "*", | ||
| ); | ||
|
|
||
| expect(window.appsmith.model).toEqual({ | ||
| test: 4, | ||
| }); | ||
|
|
||
| resolve(true); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it("should check API functions - triggerEvent", async () => { | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window.parent.postMessage as any).mockClear(); | ||
|
|
||
| window.appsmith.triggerEvent("test", { | ||
| test: 5, | ||
| }); | ||
|
|
||
| return new Promise((resolve) => { | ||
| setTimeout(() => { | ||
| expect(window.parent.postMessage).toHaveBeenCalledWith( | ||
| { | ||
| type: EVENTS.CUSTOM_WIDGET_TRIGGER_EVENT, | ||
| data: { | ||
| eventName: "test", | ||
| contextObj: { | ||
| test: 5, | ||
| }, | ||
| }, | ||
| key: expect.any(Number), | ||
| }, | ||
| "*", | ||
| ); | ||
|
|
||
| resolve(true); | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Test suite needs error handling coverage
The current test suite lacks error scenarios testing. Add test cases for:
- Invalid message formats
- Parent window communication failures
- Event handler error scenarios
- Model/UI update failures
🔗 Analysis chain
Verify test coverage metrics
Ensure adequate test coverage for edge cases and error scenarios.
Let me check the error handling coverage more specifically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for the custom widget implementation
# Find all related test files
fd -e ts -e js . | grep -i "customwidget.*test"
# Search for error handling tests
rg -l "throw|catch|error" --type ts --type js | grep -i "customwidget"
Length of output: 1564
Script:
#!/bin/bash
# Check error handling in the implementation and test files
# Search for error handling in the implementation
ast-grep --pattern 'catch ($_) { $$$ }' app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetscript.js
# Search for error test cases
rg "it\(.*error.*|test\(.*error.*|describe\(.*error.*" app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts
# Check message validation tests
rg "invalid|malformed|wrong" app/client/src/modules/ui-builder/ui/wds/WDSCustomWidget/component/customWidgetScript.test.ts
Length of output: 406
| onAdd: (widget: CustomWidgetProps, event: string) => { | ||
| const events = widget.events; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize 'events' to prevent potential errors
Ensure 'events' is initialized to an empty array to avoid issues when it's undefined.
Apply this diff:
onAdd: (widget: CustomWidgetProps, event: string) => {
- const events = widget.events;
+ const events = widget.events || [];
return {
events: [...events, event],
};
},Committable suggestion skipped: line range outside the PR's diff.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12116480051. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores