Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: Custom Widget Editor integration with AI #37257

Merged
merged 24 commits into from
Nov 11, 2024

Conversation

hetunandu
Copy link
Member

@hetunandu hetunandu commented Nov 6, 2024

Description

Adds the AI widget builder integration inside the Custom Widget Editor.
User can prompt the AI on the changes and the bot will update the code of the widget direclty.

Fixes #37250

Automation

/ok-to-test tags="@tag.Sanity"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11776093271
Commit: f48efee
Cypress dashboard.
Tags: @tag.Sanity
Spec:


Mon, 11 Nov 2024 10:29:27 UTC

Communication

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

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a feature flag for a custom AI widget builder.
    • Added a new ChatBot component for enhanced interactivity within the editor.
  • Improvements

    • Simplified layout management by removing the LayoutControls component.
    • Enhanced the TabsLayout to conditionally display an AI tab based on the feature flag.
    • Updated the Editor component to include the AI tab dynamically.
    • Expanded constants related to AI chat functionality for better integration.
  • Bug Fixes

    • Streamlined state management for layout selection in the custom widget builder context.

These updates improve user experience by integrating AI capabilities and simplifying the interface.

Copy link
Contributor

coderabbitai bot commented Nov 6, 2024

Walkthrough

The changes introduce a new feature flag release_custom_widget_ai_builder and a new React component ChatBot for integrating an AI chatbot interface within the custom widget editor. The ChatBot component facilitates communication with an iframe for dynamic updates based on user interactions. Additionally, the LayoutControls component has been removed, simplifying the layout selection process. The TabsLayout component has been modified to conditionally render an AI tab based on the feature flag, while the overall structure of the editor has been adjusted to accommodate these new functionalities.

Changes

File Path Change Summary
app/client/src/ce/entities/FeatureFlag.ts Added feature flag release_custom_widget_ai_builder and set default value to false.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx Introduced ChatBot component for AI chatbot interface with iframe integration.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/index.tsx Removed LayoutControls component from Header.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/layoutControls.tsx Deleted LayoutControls component file.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx Modified TabsLayout to conditionally set selectedTab based on feature flag for AI tab.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/index.tsx Removed conditional rendering logic for layouts, now directly rendering TabLayout.
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx Added ChatBot component to the Editor layout under a new "AI" section.
app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts Added parentEntityId to DEFAULT_CONTEXT_VALUE, removed LOCAL_STORAGE_KEYS_SELECTED_LAYOUT, and added new AI-related constants.
app/client/src/pages/Editor/CustomWidgetBuilder/types.ts Added parentEntityId to CustomWidgetBuilderContextValueType, removed selectedLayout.
app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx Removed local state for selectedLayout, added parentEntityId handling in context updates.

Assessment against linked issues

Objective Addressed Explanation
Create a function to update HTML, CSS, and Javascript inputs based on AI chat widget response (37250)
Remove the split view and tabs at the top (37250)
Add a function to post a message to the iframe with original contents when editor mode opens (37250)
Add a function to listen for messages from the iframe and update the editor (37250)

Possibly related PRs

Suggested reviewers

  • sagar-qa007
  • ankitakinger
  • albinAppsmith

"In the realm of code where features bloom,
A chatbot emerges, dispelling the gloom.
With flags flying high, the layout's anew,
Custom widgets dance, with AI in view!
Layouts simplified, controls cast away,
In this code garden, we flourish and play!" 🌼✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

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

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

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

@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

github-actions bot commented Nov 6, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11702718457.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37257.
recreate: .

Copy link

github-actions bot commented Nov 6, 2024

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

github-actions bot commented Nov 6, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11705567933.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37257.
recreate: .

Copy link

github-actions bot commented Nov 6, 2024

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

github-actions bot commented Nov 7, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11723859315.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37257.
recreate: .

Copy link

github-actions bot commented Nov 7, 2024

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

@hetunandu
Copy link
Member Author

/build-deploy-preview

Copy link

github-actions bot commented Nov 8, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11736331431.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37257.
recreate: .

Copy link

github-actions bot commented Nov 8, 2024

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

Copy link

github-actions bot commented Nov 8, 2024

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

@github-actions github-actions bot added AI All tasks related to AI IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product Integrations Product Issues related to a specific integration Query & JS Pod Issues related to the query & JS Pod Task A simple Todo labels Nov 8, 2024
@hetunandu hetunandu changed the title Feat/custom widget tab edit feat: Custom Widget Editor integration with AI Nov 8, 2024
@github-actions github-actions bot added the Enhancement New feature or request label Nov 8, 2024
@hetunandu hetunandu added the ok-to-test Required label for CI label Nov 8, 2024
@hetunandu hetunandu marked this pull request as ready for review November 8, 2024 13:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (6)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/index.tsx (1)

Line range hint 1-11: Consider simplifying Props interface

Since we're only using TabLayout now, we could rename the interface to better reflect its current usage and potentially simplify it.

-interface Props {
+interface TabLayoutProps {
   content: Array<{
     title: string;
     titleControls?: React.ReactNode;
     children: (props: ContentProps) => React.ReactNode;
   }>;
 }
app/client/src/pages/Editor/CustomWidgetBuilder/types.ts (3)

31-31: Add JSDoc comment for the new parentEntityId property.

Adding documentation will help other developers understand the purpose and usage of this property in the context of the AI widget builder integration.

+  /** ID of the parent entity containing this custom widget. Used for AI builder integration. */
   parentEntityId: string;

Line range hint 76-78: Consider addressing the TODO by properly typing the debugger logs.

The any type can be replaced with a proper type definition based on the DebuggerLog interface.

-  // TODO: Fix this the next time the file is edited
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  updateDebuggerLogs: (log: { type: string; args: any }) => void;
+  updateDebuggerLogs: (log: DebuggerLog) => void;

Line range hint 8-13: Enhance type safety for DebuggerLogType.

Consider using const assertions or an enum for better type safety and autocompletion support.

-export const DebuggerLogType = {
+export const DebuggerLogType = {
   LOG: "log",
   INFO: "info",
   WARN: "warn",
   ERROR: "error",
-};
+} as const;
+
+export type DebuggerLogTypeKeys = keyof typeof DebuggerLogType;
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1)

17-22: Props type could be more specific.

The component only uses the height prop from ContentProps. Consider creating a more specific type for this component's props.

-export const ChatBot = (props: ContentProps) => {
+type ChatBotProps = {
+  height: number;
+};
+export const ChatBot = (props: ChatBotProps) => {
app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx (1)

Line range hint 167-231: Add event listener cleanup and improve type safety.

  1. Add cleanup to prevent memory leaks
  2. Address the TODO comment by properly typing the event
  useEffect(() => {
-   // TODO: Fix this the next time the file is edited
-   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-   window.addEventListener("message", (event: any) => {
+   const handleMessage = (event: MessageEvent<{
+     type: string;
+     name?: string;
+     widgetId?: string;
+     srcDoc?: string;
+     uncompiledSrcDoc?: SrcDoc;
+     model?: Record<string, unknown>;
+     events?: string[];
+     theme?: unknown;
+   }>) => {
      switch (event.data.type) {
        // ... existing cases
      }
-   });
+   };
+   
+   window.addEventListener("message", handleMessage);
+   return () => window.removeEventListener("message", handleMessage);
  }, []);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 806c710 and 80d412c.

📒 Files selected for processing (10)
  • app/client/src/ce/entities/FeatureFlag.ts (2 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/index.tsx (0 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/layoutControls.tsx (0 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx (3 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/index.tsx (2 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (2 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts (2 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/types.ts (1 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx (5 hunks)
💤 Files with no reviewable changes (2)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/index.tsx
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Header/layoutControls.tsx
🧰 Additional context used
🪛 Biome
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx

[error] 25-25: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (15)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/index.tsx (2)

Line range hint 13-32: Implementation looks good

The changes align well with the PR objectives. The component is now more focused with simplified logic while maintaining proper width handling based on the reference state.


28-28: Verify TabLayout compatibility

Let's verify that TabLayout can handle all necessary use cases previously managed by SplitLayout.

app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (3)

11-11: LGTM: Clean import statement

The ChatBot import follows React conventions and maintains good code organization by keeping related components co-located.


Line range hint 21-44: Reminder: Split view removal pending

The PR objectives mention removing the split view and tabs, but this change is not reflected in the current implementation. The Layout component still uses the traditional structure.

#!/bin/bash
# Description: Check for split view related code that needs to be removed

# Search for split view related components and usage
echo "Checking for split view components..."
rg -l "SplitView|TabsLayout" 

Consider:

  1. Removing the split view component
  2. Updating the Layout structure to match the new design
  3. Ensuring the ChatBot integration works with the new layout

37-40: ⚠️ Potential issue

Verify feature flag implementation

The AI section should be conditionally rendered based on the release_custom_widget_ai_builder feature flag mentioned in the summary.

Consider wrapping the AI section with the feature flag:

+ const isAIEnabled = useFeatureFlag("release_custom_widget_ai_builder");
  <Layout
    content={[
      // ... other sections
-     {
-       title: "AI",
-       children: (props: ContentProps) => <ChatBot {...props} />,
-     },
+     ...(isAIEnabled
+       ? [{
+           title: "AI",
+           children: (props: ContentProps) => <ChatBot {...props} />,
+         }]
+       : []),
    ]}
  />
app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts (2)

63-65: LGTM! Constants align with AI integration requirements

The new constants follow the established naming pattern and provide clear identifiers for the AI chat functionality.


19-19: LGTM! Verify parent entity relationship handling

The addition of parentEntityId looks good, but let's verify its usage in the codebase.

✅ Verification successful

parentEntityId usage verified and properly implemented

The parentEntityId is correctly used throughout the codebase for:

  • Parent-child relationship tracking in actions and queries
  • Navigation and routing in the editor
  • Context management in the Custom Widget Builder

The empty string default in DEFAULT_CONTEXT_VALUE aligns with its usage patterns, particularly in:

  • useCustomBuilder.tsx where it's consumed via useParentEntityInfo
  • ChatBot.tsx where it's used to create unique instance IDs
  • Various entity explorer components for proper context management
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for parentEntityId usage to ensure proper implementation
rg "parentEntityId" --type ts

Length of output: 11559

app/client/src/pages/Editor/CustomWidgetBuilder/types.ts (1)

Line range hint 1-85: Interface structure looks good!

The removal of layout-related properties and the addition of parentEntityId align well with the PR objectives. The interface structure maintains good separation of concerns between values and functions.

app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1)

1-16: Consider adding origin validation for security.

The code uses postMessage communication which could be vulnerable to cross-origin attacks. Consider importing and using a constant for allowed origins.

app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx (2)

6-8: LGTM: Clean import additions

The new imports support the AI feature integration and conditional styling requirements.


98-101: Consider performance impact of forceMount

While forceMount is necessary for maintaining editor state, it keeps all tab content mounted simultaneously. This could impact performance with multiple complex tabs.

Let's check how many tabs typically exist:

Consider implementing a cleanup mechanism for inactive tab content if memory usage becomes a concern.

app/client/src/ce/entities/FeatureFlag.ts (1)

44-44: LGTM! Verify feature flag integration.

The feature flag implementation follows the naming convention and is properly configured with a default false value.

Let's verify the feature flag usage:

Also applies to: 81-81

app/client/src/pages/Editor/CustomWidgetBuilder/useCustomBuilder.tsx (3)

19-20: LGTM: Clean hook integration.

The new hooks are properly integrated and follow the dependency chain (editorType → parentEntityId).

Also applies to: 32-33


153-153: LGTM: Proper dependency management.

Dependencies are correctly updated following the removal of layout-related functionality.

Also applies to: 162-162


179-179: LGTM: Consistent state updates.

The parentEntityId is consistently handled in both READY_ACK and RESUME events.

Also applies to: 217-217

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (1)

21-46: Consider extracting tab configurations

The tabs configuration looks good, but consider extracting the default tabs array to a separate constant for better maintainability.

+const DEFAULT_EDITOR_TABS = [
+  {
+    title: "HTML",
+    children: (props: ContentProps) => <HTMLEditor {...props} />,
+  },
+  {
+    title: "Style",
+    titleControls: <TitleControls />,
+    children: (props: ContentProps) => <StyleEditor {...props} />,
+  },
+  {
+    title: "Javascript",
+    children: (props: ContentProps) => <JSEditor {...props} />,
+  },
+];

 const tabs = useMemo(() => {
-  const defaultTabs = [/*...*/];
+  const defaultTabs = [...DEFAULT_EDITOR_TABS];
   if (isAIEnabled) {
     defaultTabs.push({/*...*/});
   }
   return defaultTabs;
 }, [isAIEnabled]);
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (3)

19-19: Consider using a more specific type for the timestamp ref.

For better type safety and clarity, consider using useRef<number | null>(null) or adding a comment explaining the timestamp usage.


24-40: Consider using a proper debounce utility.

Replace the manual timestamp check with lodash's debounce utility for more reliable throttling:

-  const lastUpdateFromBot = useRef<number>(0);
+  const handleSrcDocUpdates = useCallback(
+    debounce(() => {
       if (ref.current && ref.current.contentWindow && uncompiledSrcDoc) {
         ref.current.contentWindow.postMessage(
           {
             html_code: uncompiledSrcDoc.html,
             css_code: uncompiledSrcDoc.css,
             js_code: uncompiledSrcDoc.js,
             chatType: CUSTOM_WIDGET_AI_CHAT_TYPE,
           },
           "*",
         );
       }
-    }, [uncompiledSrcDoc]);
+    }, 100), [uncompiledSrcDoc]);
🧰 Tools
🪛 Biome

[error] 29-29: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


44-46: Improve type safety with a discriminated union type.

Consider creating a more specific type for the message data:

type ChatBotMessage = 
  | { type: 'code'; html_code?: string; css_code?: string; js_code?: string }
  | { type: 'init' };
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 80d412c and cffb595.

📒 Files selected for processing (2)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx

[error] 29-29: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (4)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (3)

Line range hint 48-58: LGTM! Clean and maintainable component structure

The component rendering is well-organized and follows React best practices.


17-19: Verify feature flag definition

The feature flag implementation looks good, but let's ensure it's properly defined in the feature flags configuration.

✅ Verification successful

Feature flag is properly defined and configured

The feature flag release_custom_widget_ai_builder is correctly defined in the feature flags configuration with:

  • Proper type definition in FEATURE_FLAG constant
  • Default value set to false in DEFAULT_FEATURE_FLAG_VALUE
  • Follows the naming convention for release-controlled features
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check feature flag definition
rg "release_custom_widget_ai_builder" -A 2

Length of output: 1086


Script:

#!/bin/bash
# Let's check the feature flag type definition and configuration
cat app/client/src/ce/entities/FeatureFlag.ts

Length of output: 4111


11-13: Verify ChatBot component implementation

The imports look correct, but let's ensure the ChatBot component is properly implemented.

app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1)

90-92: Verify the bot URL configuration.

Let's ensure the bot URL constant is properly configured across the codebase.

✅ Verification successful

Let me verify the chat type constant since it's used in the URL construction.


Bot URL configuration is properly integrated

The URL construction is correctly implemented with:

  • Constant defined in constants.ts with proper parameter handling
  • Chat type constant properly defined and used
  • URL parameters properly encoded including origin and instance ID
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CUSTOM_WIDGET_AI_BOT_URL usage and configuration
echo "Searching for bot URL configuration..."
rg "CUSTOM_WIDGET_AI_BOT_URL" -A 2

echo "Checking for environment variables used in the URL..."
rg "CUSTOM_WIDGET_AI" .env* || echo "No direct environment variables found"

Length of output: 1393


Script:

#!/bin/bash
# Search for CUSTOM_WIDGET_AI_CHAT_TYPE definition and usage
echo "Searching for chat type constant..."
rg "CUSTOM_WIDGET_AI_CHAT_TYPE" -A 2

# Check if there are any other related constants
echo "Checking for related constants..."
rg "CUSTOM_WIDGET.*_TYPE" -A 2

Length of output: 2683

@hetunandu
Copy link
Member Author

/build-deploy-preview

@hetunandu hetunandu enabled auto-merge (squash) November 11, 2024 05:26
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11772697222.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 37257.
recreate: .

Copy link

Deploy-Preview-URL: https://ce-37257.dp.appsmith.com

Copy link
Contributor

@rahulbarwal rahulbarwal left a comment

Choose a reason for hiding this comment

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

Requesting minor changes
Nit: what is the use of this trash icon when user has not typed anything?

image


const handleSrcDocUpdates = useCallback(() => {
// Don't send updates back to bot if the last update came from the bot within the last 100ms
if (Date.now() - lastUpdateFromBot.current < 100) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hard coding;100 ms can be made a constant variable for readability

const [selectedTab, setSelectedTab] = useLocalStorageState<string>(
LOCAL_STORAGE_KEYS,
tabs[0].title,
isDefaultAITab ? "AI" : tabs[0].title,
Copy link
Contributor

Choose a reason for hiding this comment

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

AI here.. should come from messages file right?

const tabs = useMemo(() => {
const defaultTabs = [
{
title: "HTML",
Copy link
Contributor

Choose a reason for hiding this comment

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

These titles, should be coming from messages file

import styles from "./styles.module.css";
import { CustomWidgetBuilderContext } from "../..";
import TabLayout from "./TabsLayout";
import SplitLayout from "./SplitLayout";
Copy link
Contributor

Choose a reason for hiding this comment

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

I could not find any other ref of this SplitLayout component. I think its safe to remove this component altogether.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (6)
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (1)

18-20: Consider the lifecycle of this feature flag.

As this appears to be a permanent feature addition rather than a temporary rollout, we should plan for eventual flag removal to avoid technical debt.

app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts (2)

24-24: Document the purpose of parentEntityId

Please add a comment explaining the purpose and usage of this new context property.


75-75: Consider documenting the debounce timeout choice

The 100ms debounce timeout seems reasonable, but please document why this specific value was chosen and any performance implications.

app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (3)

18-23: Add type safety improvements.

Consider adding type guards and null checks for context values:

+ type ChatBotMessage = {
+   html_code?: string;
+   css_code?: string;
+   js_code?: string;
+ };

  export const ChatBot = (props: ContentProps) => {
    const ref = useRef<HTMLIFrameElement>(null);
    const lastUpdateFromBot = useRef<number>(0);
+   const context = useContext(CustomWidgetBuilderContext);
+   
+   if (!context) {
+     throw new Error("ChatBot must be used within CustomWidgetBuilderContext");
+   }
+   
    const { bulkUpdate, parentEntityId, uncompiledSrcDoc, widgetId } = context;

27-32: Extract debounce condition to a constant.

The debounce condition would be more maintainable if extracted to a descriptive constant.

+ const DEBOUNCE_CONDITION = (lastUpdate: number) =>
+   Date.now() - lastUpdate < CUSTOM_WIDGET_AI_BOT_MESSAGE_RESPONSE_DEBOUNCE_TIMEOUT;

  // Don't send updates back to bot if the last update came from the bot within the last 100ms
- if (
-   Date.now() - lastUpdateFromBot.current <
-   CUSTOM_WIDGET_AI_BOT_MESSAGE_RESPONSE_DEBOUNCE_TIMEOUT
- ) {
+ if (DEBOUNCE_CONDITION(lastUpdateFromBot.current)) {
    return;
  }

94-98: Optimize instanceId memoization.

The instanceId should be memoized to prevent unnecessary recalculations:

- const instanceId = `${widgetId}-${parentEntityId}`;

- const srcUrl = useMemo(() => {
-   return CUSTOM_WIDGET_AI_BOT_URL(instanceId);
- }, [instanceId]);

+ const instanceId = useMemo(
+   () => `${widgetId}-${parentEntityId}`,
+   [widgetId, parentEntityId]
+ );

+ const srcUrl = useMemo(
+   () => CUSTOM_WIDGET_AI_BOT_URL(instanceId),
+   [instanceId]
+ );
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cffb595 and f48efee.

📒 Files selected for processing (7)
  • app/client/src/ce/constants/messages.ts (1 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx (1 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/SplitLayout/index.tsx (0 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/SplitLayout/styles.module.css (0 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx (3 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/index.tsx (2 hunks)
  • app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts (3 hunks)
💤 Files with no reviewable changes (2)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/SplitLayout/index.tsx
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/SplitLayout/styles.module.css
✅ Files skipped from review due to trivial changes (1)
  • app/client/src/ce/constants/messages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/client/src/pages/Editor/CustomWidgetBuilder/Editor/Layouts/TabsLayout/index.tsx
🧰 Additional context used
🪛 Biome
app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx

[error] 35-35: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
app/client/src/pages/Editor/CustomWidgetBuilder/constants.ts (3)

1-4: LGTM: Clean imports

The imports are properly scoped and align with the new AI tab functionality.


72-73: Security concerns in URL generation remain unaddressed


77-82: LGTM: Well-structured tab configuration

The tab configuration follows i18n best practices. Let's verify the message keys exist.

✅ Verification successful

Tab configuration is properly internationalized

The message keys are correctly defined in app/client/src/ce/constants/messages.ts and properly used in the tab configuration. The implementation follows i18n best practices with all required keys present.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that all tab title message keys are defined
rg -l "CUSTOM_WIDGET_BUILDER_TAB_TITLE" "ee/constants/messages"

Length of output: 123


Script:

#!/bin/bash
# Search for message keys in the entire codebase
rg "CUSTOM_WIDGET_BUILDER_TAB_TITLE"

# Also search for createMessage usage to find the constants file location
rg "createMessage.*CUSTOM_WIDGET_BUILDER_TAB_TITLE"

Length of output: 1239


Script:

#!/bin/bash
# Let's check the actual message definitions in the constants file
cat app/client/src/ce/constants/messages.ts | rg -A5 "CUSTOM_WIDGET_BUILDER_TAB_TITLE ="

Length of output: 232

if (isAIEnabled) {
defaultTabs.push({
title: CUSTOM_WIDGET_BUILDER_TABS.AI,
children: (props: ContentProps) => <ChatBot {...props} />,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

ChatBot component is using more props than defined in ContentProps

The ChatBot component is using props (bulkUpdate, parentEntityId, uncompiledSrcDoc, widgetId) from context that aren't part of the ContentProps interface, which only defines height, width, and onChange. This mismatch should be addressed.

  • Consider creating a dedicated props interface for the ChatBot component that extends ContentProps with the additional required props
  • Alternatively, if these context values are sufficient, remove the ContentProps typing
🔗 Analysis chain

Verify ChatBot component prop requirements.

The ChatBot component receives the same ContentProps type as code editors. Ensure this is intentional and that ChatBot actually requires all these props.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check ChatBot component's prop types
ast-grep --pattern 'interface $_ {
  $$$
}' ./app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx

Length of output: 122


Script:

#!/bin/bash
# Let's try a different approach to find the ChatBot component and its props
rg -A 10 "const ChatBot.*=" ./app/client/src/pages/Editor/CustomWidgetBuilder/Editor/ChatBot/ChatBot.tsx

# Also check the ContentProps type definition
rg -A 10 "type ContentProps|interface ContentProps" ./app/client/src/pages/Editor/CustomWidgetBuilder/Editor/

Length of output: 1209

Comment on lines +68 to +76
if (isObject(event.data)) {
lastUpdateFromBot.current = Date.now();

bulkUpdate({
html: event.data.html_code || "",
css: event.data.css_code || "",
js: event.data.js_code || "",
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type validation for message data.

Consider adding type validation to ensure message data is properly structured:

+ const isValidMessageData = (
+   data: unknown
+ ): data is { html_code?: string; css_code?: string; js_code?: string } => {
+   if (!isObject(data)) return false;
+   const { html_code, css_code, js_code } = data as any;
+   return (
+     (html_code === undefined || typeof html_code === "string") &&
+     (css_code === undefined || typeof css_code === "string") &&
+     (js_code === undefined || typeof js_code === "string")
+   );
+ };

- if (isObject(event.data)) {
+ if (isValidMessageData(event.data)) {
    lastUpdateFromBot.current = Date.now();
    bulkUpdate({
      html: event.data.html_code || "",
      css: event.data.css_code || "",
      js: event.data.js_code || "",
    });
  }
📝 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.

Suggested change
if (isObject(event.data)) {
lastUpdateFromBot.current = Date.now();
bulkUpdate({
html: event.data.html_code || "",
css: event.data.css_code || "",
js: event.data.js_code || "",
});
}
const isValidMessageData = (
data: unknown
): data is { html_code?: string; css_code?: string; js_code?: string } => {
if (!isObject(data)) return false;
const { html_code, css_code, js_code } = data as any;
return (
(html_code === undefined || typeof html_code === "string") &&
(css_code === undefined || typeof css_code === "string") &&
(js_code === undefined || typeof js_code === "string")
);
};
if (isValidMessageData(event.data)) {
lastUpdateFromBot.current = Date.now();
bulkUpdate({
html: event.data.html_code || "",
css: event.data.css_code || "",
js: event.data.js_code || "",
});
}

@hetunandu hetunandu merged commit d8d0d1a into release Nov 11, 2024
45 checks passed
@hetunandu hetunandu deleted the feat/custom-widget-tab-edit branch November 11, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI All tasks related to AI Enhancement New feature or request IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product Integrations Product Issues related to a specific integration ok-to-test Required label for CI Query & JS Pod Issues related to the query & JS Pod Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Update functionality for the Custom widget editor using an AI assistant
2 participants