Conversation
WalkthroughThe changes introduce several new features and enhancements. A new asynchronous icon is added in the design system, and sidebar components receive an extra prop for an additional title button with corresponding type and CSS updates. A comprehensive visualization feature is implemented, including new React components, hooks for generating and saving visualizations, and API methods to support these operations. Additional updates include new type definitions, a debugger tab key, and extended feature flag capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant VC as Visualization Component
participant GHook as useGenerateVisualization
participant SHook as useSaveVisualization
participant API as ActionAPI
U->>VC: Initiate visualization generation (enter prompt)
VC->>GHook: execute(prompt, data)
GHook->>API: POST generateVisualization(actionId, prompt, data)
API-->>GHook: Return visualization result
GHook->>VC: Update visualization state
U->>VC: Trigger save operation
VC->>SHook: execute(elements)
SHook->>API: PATCH saveVisualization(actionId, elements)
API-->>SHook: Return save response
SHook->>VC: Confirm saved state
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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
|
github-actions
bot
added
AI Agents
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13810603025. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (1)
47-65: Consider conditional rendering for extraTitleButton.The implementation uses a Flex container to group both buttons, which is appropriate. However, unlike the expand button which has a conditional check,
extraTitleButtonis rendered directly without any conditional checks. While React handles undefined/null values, consider adding a Boolean check for consistency and to avoid potential layout issues.<Flex> - {extraTitleButton} + {Boolean(extraTitleButton) && extraTitleButton} {!isMobile && ( <Button className={styles.sidebarHeaderExpandButton} color="neutral" icon={ state === "full-width" ? "arrows-diagonal-minimize" : "arrows-diagonal-2" } onPress={() => setState(state === "full-width" ? "expanded" : "full-width") } variant="ghost" /> )} </Flex>app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx (1)
4-22: Component implementation is sound.The LoadingOverlay component correctly implements a full-viewport loading indicator with appropriate styling.
Consider enhancing accessibility by adding ARIA attributes:
<Flex alignItems="center" backgroundColor="color-mix(in srgb, var(--ads-v2-color-gray-800) 80%, transparent);" bottom="0" flexDirection="column" justifyContent="center" left="0" position="absolute" right="0" top="0" + role="alert" + aria-live="polite" >app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx (1)
1-43: Good implementation, consider adding accessibility attributesThe component is well-structured with proper use of React.memo for optimization. The button implementation is clean and follows best practices with key properties correctly set.
Consider enhancing accessibility by adding aria attributes to the buttons. Additionally, extracting the suggestions array to a constants file might improve maintainability if these suggestions need to be modified or reused elsewhere.
<Button key={suggestion.label} kind="secondary" onClick={() => onApply(suggestion.text)} + aria-label={`Create a ${suggestion.label.toLowerCase()}`} > {suggestion.label} </Button>app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx (1)
19-47: Improve accessibility and fix spelling in placeholderThe component implementation is clean overall, but has a few areas for improvement.
- Add a proper label to the input for accessibility
- Fix spelling in placeholder (visualisation → visualization)
<PromptForm onSubmit={(e) => { e.preventDefault(); onSubmit(); }} > + <label htmlFor="visualization-prompt" className="sr-only"> + Visualization prompt + </label> <Input isDisabled={isDisabled} onChange={onChange} - placeholder="Describe the data visualisation you want" + placeholder="Describe the data visualization you want" + id="visualization-prompt" size="md" value={value} />app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts (1)
7-41: Enhance error handling with specific error messagesThe hook implementation follows good practices with proper state management and dependency handling in useCallback. However, error handling could be improved.
Consider enhancing the error state to include more detailed information rather than just a boolean flag:
- const [hasError, setHasError] = useState(false); + const [error, setError] = useState<{ hasError: boolean; message?: string }>({ + hasError: false, + }); const execute = useCallback( async (elements: VisualizationElements) => { setIsLoading(true); - setHasError(false); + setError({ hasError: false }); try { const response = await ActionAPI.saveVisualization(actionId, elements); dispatch( updateActionProperty({ id: actionId, field: "visualization", value: response.data, }), ); } catch (error) { - setHasError(true); + setError({ + hasError: true, + message: error instanceof Error ? error.message : "Failed to save visualization" + }); } finally { setIsLoading(false); } }, [actionId], ); return { execute, isLoading, - hasError, + error, };This would provide more context to the UI about what went wrong, enabling better error messaging for users.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx (2)
33-33: Consider removing or parameterizing the hardcoded height.
This TODO can be addressed by introducing a more flexible layout approach, such as using a parent container or adjusting the layout dynamically based on content size.
45-55: Double-check prompt input usability.
Ensure users clearly understand when the prompt is disabled. Consider surfacing a tooltip or message explaining why prompt entry is disabled when there is no response or while saving.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (1)
91-108: Leverage optional chaining for cleaner checks (lines 94-95).
Safely reduce nested checks by usingiframeRef.current?.contentWindow. This eliminates the risk of null dereferences and aligns with modern JavaScript best practices.- if ( - iframeRef.current && - iframeRef.current.contentWindow && - isIframeReady - ) { + if (iframeRef.current?.contentWindow && isIframeReady) { ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts (1)
30-57: Ensure proper cancellation or queueing of visualization requests.
In the current logic, multiple consecutive calls toexecutecould overlap. Consider adding an abort mechanism or queue system if performance or concurrency becomes a concern.app/client/src/api/ActionAPI.tsx (2)
165-185: Consider adding JSDoc comments and timeout parameter.The method implementation looks good, but would benefit from documentation. Also, the hardcoded timeout value (60000ms) should be moved to a constant or made configurable.
+ /** + * Generates a visualization for an action with the given prompt and data. + * @param actionId - ID of the action to visualize + * @param prompt - User prompt for visualization generation + * @param data - Data to be visualized + * @returns Promise with visualization HTML, CSS, JS and any error + */ static async generateVisualization( actionId: string, prompt: string, data: unknown, ): Promise< AxiosPromise<{ result: { html: string; css: string; js: string; error: string }; }> > { return API.post( `${ActionAPI.url}/${actionId}/visualize`, { prompt, data, }, {}, { - timeout: 60000, // 1 minute + timeout: VISUALIZATION_GENERATION_TIMEOUT_MS, }, ); }You'll need to define the constant at the top of the file or import it from a constants file.
187-206: Add documentation and consider cancelation support.The method is well implemented, but lacks documentation. Since visualization data might be large, consider adding cancelation token support similar to the
updateActionmethod.+ /** + * Saves an existing visualization for an action. + * @param actionId - ID of the action to save visualization for + * @param elements - Object containing HTML, CSS and JS for the visualization + * @returns Promise with saved visualization elements and any error + */ static async saveVisualization( actionId: string, elements: { html: string; css: string; js: string; }, ): Promise< AxiosPromise< ApiResponse<{ result: { html: string; css: string; js: string; error: string }; }> > > { + if (ActionAPI.visualizationSaveCancelTokenSource) { + ActionAPI.visualizationSaveCancelTokenSource.cancel(); + } + + ActionAPI.visualizationSaveCancelTokenSource = axios.CancelToken.source(); return API.patch(`${ActionAPI.url}/${actionId}/visualize`, { existing: { result: elements, }, }); }You'll also need to add the token source property to the class:
static visualizationSaveCancelTokenSource: CancelTokenSource;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/src/assets/images/no-visualization.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx(4 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts(1 hunks)app/client/src/api/ActionAPI.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/constants.ts(1 hunks)app/client/src/entities/Action/index.ts(2 hunks)app/client/src/layoutSystems/anvil/integrations/selectors.ts(1 hunks)app/client/src/utils/hooks/useFeatureFlagOverride.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (21)
app/client/src/utils/hooks/useFeatureFlagOverride.ts (1)
19-19: Feature flag addition looks goodThe new feature flag follows the naming convention of existing flags and is properly integrated into the overridable features array.
app/client/src/layoutSystems/anvil/integrations/selectors.ts (1)
13-15: Selector implementation is correctThe new selector
getReleaseFnCallingEnabledfollows the established pattern of other feature flag selectors in the file and properly uses the previously added feature flag.app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (1)
27-27: Well-typed enhancement for sidebar customization.Adding this optional
extraTitleButtonprop of typeReactNodeprovides a clean way to extend the sidebar's functionality with an additional title button component.app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (1)
173-173: CSS adjustment to accommodate the new button.Reduced horizontal margin from
var(--sizing-14)tovar(--sizing-4)provides appropriate spacing for the additional title button that will be rendered. This change coordinates well with the TypeScript interface enhancement.app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (2)
17-17: Properly destructures the new prop from props object.The addition of
extraTitleButtonto the destructured props is consistent with the interface update.
51-51: Correctly passes the new prop to SidebarContent.The
extraTitleButtonprop is properly passed down to the SidebarContent component where it will be rendered.app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (2)
11-13: New asynchronous icon added to design system.The ArrowGoForwardLineIcon is imported using the established pattern with the importRemixIcon helper function, maintaining consistency with other icon imports.
1184-1184: Icon correctly added to lookup dictionary.The new icon is properly registered in the ICON_LOOKUP object with the key "arrow-go-forward", making it accessible through the IconProvider component.
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (3)
2-2: Appropriate import of ReactNode type.The addition of ReactNode type import is correctly used for typing the new prop.
12-12: Well-defined optional property added to interface.The new optional property
extraTitleButtonis properly typed as ReactNode and follows the interface's existing pattern.
21-21: Props destructuring correctly updated.The destructuring statement has been appropriately updated to include the new
extraTitleButtonprop.app/client/src/components/editorComponents/Debugger/constants.ts (1)
9-9: LGTM. Enum addition follows existing pattern.The added VISUALIZATION_TAB entry maintains the naming convention pattern of the existing enum values.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts (1)
1-1: Barrel file implementation looks good.Clean barrel export pattern that will simplify imports across the application.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx (1)
5-18: Component implementation looks good.The EmptyVisualization component is correctly implemented with appropriate alt text for the image and clear user messaging.
app/client/src/entities/Action/index.ts (2)
127-131: LGTM: New interface for visualization elementsThe VisualizationElements interface is well-defined with clear property types.
166-168:Details
❓ Verification inconclusive
Consider validating visualization input before execution
The visualization property is correctly added to the BaseAction interface. However, there might be security implications when executing user-provided JS code.
Make sure there's validation and sanitization for the JS content before it's executed in the browser to prevent potential XSS attacks.
🏁 Script executed:
#!/bin/bash # Check if there's any validation or sanitization for the visualization JS rg -A 3 -B 3 "sanitize|validate|xss|security" --glob "*.{ts,tsx}" app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Length of output: 159
Action Required: Confirm Validation/Sanitization for Visualization JS
We verified the PluginActionEditor visualization folder using the provided search command, and no built-in sanitization or validation logic was found. Please ensure that any user-supplied JS content for visualization is properly checked before execution to mitigate potential XSS risks. If centralized sanitization exists elsewhere, please reference it; otherwise, add explicit input validation/sanitization.
- Verify in centralized modules or elsewhere in the codebase for functions (e.g., DOMPurify, sanitizeHtml) that might secure this input.
- If validation is missing, update the logic in the visualization execution path (e.g., in PluginActionEditor components or the centralized execution module).
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx (1)
13-17: Move styled component outside of the component definitionThe styled component is defined within the file but outside the component, which is good. The styling looks clean and uses design tokens.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx (2)
57-79: Good navigation button design.
Having dedicated navigation functions for previous/next results promotes clarity. No issues found here.
81-95: Validate saving logic when elements are undefined.
Although you disable the button if there are no elements, you might benefit from an additional assertion or error message if an undefined state unexpectedly occurs. This defensive check can help diagnose unexpected edge cases.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (1)
28-34: Confirm Babel transform configuration.
Be sure that “react” is the correct preset for your compiled JS. If you intend to support additional language features (e.g., TypeScript annotations), consider extending your Babel configuration accordingly.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts (1)
16-21: Avoid negative indexing with clamping.
The clampIndex function correctly ensures valid indices. No changes needed, but keep in mind scenarios where the user might rapidly update the results array.
| useEffect( | ||
| function setupIframeMessageHandler() { | ||
| const handler = (event: MessageEvent) => { | ||
| const iframeWindow = | ||
| iframeRef.current?.contentWindow || | ||
| iframeRef.current?.contentDocument?.defaultView; | ||
|
|
||
| if (event.source === iframeWindow) { | ||
| iframeRef.current?.contentWindow?.postMessage( | ||
| { | ||
| type: EVENTS.CUSTOM_WIDGET_MESSAGE_RECEIVED_ACK, | ||
| key: event.data.key, | ||
| success: true, | ||
| }, | ||
| "*", | ||
| ); | ||
|
|
||
| const message = event.data; | ||
|
|
||
| switch (message.type) { | ||
| case EVENTS.CUSTOM_WIDGET_READY: | ||
| setIsIframeReady(true); | ||
| iframeRef.current?.contentWindow?.postMessage( | ||
| { | ||
| type: EVENTS.CUSTOM_WIDGET_READY_ACK, | ||
| model: { | ||
| data, | ||
| }, | ||
| ui: { | ||
| width: 0, | ||
| height: 0, | ||
| }, | ||
| theme, | ||
| }, | ||
| "*", | ||
| ); | ||
|
|
||
| break; | ||
| case "CUSTOM_WIDGET_CONSOLE_EVENT": | ||
| if (message.data.type === "error") { | ||
| throw new Error(message.data.args); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("message", handler, false); | ||
|
|
||
| return () => window.removeEventListener("message", handler, false); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robust handling of inter-iframe communication.
The switch cases neatly handle event types. However, if new event types are added, consider extracting the logic into a separate function or dictionary-based handler to keep this effect concise and scalable.
znamenskii-ilia
force-pushed
the
feat/39554-resp-viz
branch
from
b593b16 to
a7f06bb
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13810704310. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx (2)
8-25: Consider verifying uniqueness of the 'label' keys.
If you plan to add more suggestions dynamically, ensuring unique keys prevents potential rendering issues.
27-43: Minor note on consistent styling.
Consider adding a brief doc comment in theSuggestionButtonscomponent to clarify the usage of theonApplyprop. This can be helpful for future contributors.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx (3)
22-33: Remove or parametrize the hardcoded height.
TheTODOcomment about removing the hardcoded height is a good reminder. Consider passing it as a prop or using more flexible styling to avoid layout constraints.
45-55: Add graceful handling if generation fails.
Currently, ifgenerateVisualization.executefails, there’s no user-facing feedback beyond stopping the loading state. Consider displaying an error or fallback to guide users.
80-95: Provide feedback on save failures.
When save is triggered, there's no error handling ifsaveVisualization.executeencounters an issue. Adding toast notifications or an alert helps with user experience.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (1)
94-96: Use optional chaining for cleaner checks.
You can simplify the conditions by using optional chaining.- if ( - iframeRef.current && - iframeRef.current.contentWindow && - isIframeReady - ) { + if (iframeRef.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/api/ActionAPI.tsx (2)
165-185: Approve with suggestions: Consider adding JSDoc commentsThe implementation of
generateVisualizationlooks good and follows the class pattern. The 60-second timeout is appropriate for a potentially resource-intensive operation.Consider adding JSDoc comments to document the method's purpose, parameters, and return value for better maintainability.
187-206: Approve with suggestions: Add input validationThe implementation of
saveVisualizationis well-structured. However, there's no validation to ensure that theelementsobject contains valid HTML, CSS, and JS content before sending to the server.Consider adding client-side validation or error handling for malformed content to prevent potential issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
app/client/src/assets/images/no-visualization.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx(4 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts(1 hunks)app/client/src/api/ActionAPI.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/constants.ts(1 hunks)app/client/src/entities/Action/index.ts(2 hunks)app/client/src/layoutSystems/anvil/integrations/selectors.ts(1 hunks)app/client/src/utils/hooks/useFeatureFlagOverride.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- app/client/src/utils/hooks/useFeatureFlagOverride.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx
- app/client/src/components/editorComponents/Debugger/constants.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx
- app/client/src/entities/Action/index.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx
- app/client/src/layoutSystems/anvil/integrations/selectors.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts
- app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (2)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx (1)
1-3: Looks great overall!
The imports and memo usage appear straightforward and optimized.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (1)
75-77: Be cautious when throwing errors in event handlers.
Although theErrorBoundaryin the parent handles this, throwing within an event listener can still affect debugging. Consider a more controlled error flow if you need advanced logs or multiple event sources.
znamenskii-ilia
force-pushed
the
feat/39554-resp-viz
branch
from
a7f06bb to
1a5b458
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13810773671. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/src/ce/entities/FeatureFlag.ts (1)
57-103: Consider adding documentation for the flagWhile the flag follows naming conventions, consider adding a brief comment about what functionality this flag controls. This would help other developers understand its purpose without having to look elsewhere in the codebase.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx (2)
33-33: Address the TODO comment in production code.Remove this TODO comment and implement a proper height calculation or CSS layout solution instead of hardcoding the height value.
46-96: Consider extracting button logic to reduce duplication.The buttons in this component share similar disabled conditions and state management. Consider extracting a custom component or utility function to manage these common attributes.
- <Flex gap="spaces-3"> - <PromptInput - isDisabled={!response || saveVisualization.isLoading} - isLoading={generateVisualization.isLoading} - onChange={setPrompt} - onSubmit={async () => - generateVisualization.execute(prompt, response) - } - value={prompt} - /> - <Button - isDisabled={ - generateVisualization.isLoading || - saveVisualization.isLoading || - !generateVisualization.hasPrevious - } - isIconButton - kind="secondary" - onClick={generateVisualization.previous} - size="md" - startIcon="arrow-go-back" - /> - <Button - isDisabled={ - generateVisualization.isLoading || - saveVisualization.isLoading || - !generateVisualization.hasNext - } - isIconButton - kind="secondary" - onClick={generateVisualization.next} - size="md" - startIcon="arrow-go-forward" - /> - <Button - isDisabled={ - generateVisualization.isLoading || - saveVisualization.isLoading || - !generateVisualization.elements - } - isLoading={saveVisualization.isLoading} - kind="secondary" - onClick={async () => - generateVisualization.elements && - saveVisualization.execute(generateVisualization.elements) - } - size="md" - > - Save - </Button> + <Flex gap="spaces-3"> + <PromptInput + isDisabled={!response || saveVisualization.isLoading} + isLoading={generateVisualization.isLoading} + onChange={setPrompt} + onSubmit={async () => + generateVisualization.execute(prompt, response) + } + value={prompt} + /> + {renderNavigationButtons(generateVisualization, saveVisualization)} + {renderSaveButton(generateVisualization, saveVisualization)} </Flex>app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (2)
93-105: Apply optional chaining for cleaner code.The conditional check can be simplified using optional chaining for better readability.
- if ( - iframeRef.current && - iframeRef.current.contentWindow && - isIframeReady - ) { + if (iframeRef.current?.contentWindow && isIframeReady) {🧰 Tools
🪛 Biome (1.9.4)
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
75-77: Improve error handling in visualization errors.Currently throwing a generic Error with message.data.args. Consider using a more specific error type or providing more context about the error source for better debugging.
- if (message.data.type === "error") { - throw new Error(message.data.args); - } + if (message.data.type === "error") { + throw new Error(`Visualization script error: ${message.data.args}`); + }app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts (1)
31-57: Add more specific error handling.The current error handling only sets a boolean flag. Consider capturing the error details for more informative user feedback or logging.
- try { - const response = await ActionAPI.generateVisualization( - actionId, - prompt, - data, - ); - - setResults((result) => [ - ...result, - { - js: response.data.result.js, - css: response.data.result.css, - html: response.data.result.html, - }, - ]); - } catch (error) { - setHasError(true); - } finally { + try { + const response = await ActionAPI.generateVisualization( + actionId, + prompt, + data, + ); + + setResults((result) => [ + ...result, + { + js: response.data.result.js, + css: response.data.result.css, + html: response.data.result.html, + }, + ]); + } catch (error) { + setHasError(true); + console.error("Visualization generation failed:", error); + // Optionally store error message for display to the user + } finally {app/client/src/api/ActionAPI.tsx (2)
165-185: Consider using a constant for timeout value.The 60000ms timeout is hardcoded. Consider extracting this to a named constant for better maintainability.
+const VISUALIZATION_GENERATION_TIMEOUT_MS = 60000; // 1 minute static async generateVisualization( actionId: string, prompt: string, data: unknown, ): Promise< AxiosPromise<{ result: { html: string; css: string; js: string; error: string }; }> > { return API.post( `${ActionAPI.url}/${actionId}/visualize`, { prompt, data, }, {}, { - timeout: 60000, // 1 minute + timeout: VISUALIZATION_GENERATION_TIMEOUT_MS, }, ); }
187-206: Consider adding error handling for malformed visualization elements.The saveVisualization method takes elements with html, css, and js properties but doesn't validate their format. Consider adding validation or error handling for malformed inputs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
app/client/src/assets/images/no-visualization.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx(4 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts(1 hunks)app/client/src/api/ActionAPI.tsx(1 hunks)app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/components/editorComponents/Debugger/constants.ts(1 hunks)app/client/src/entities/Action/index.ts(2 hunks)app/client/src/layoutSystems/anvil/integrations/selectors.ts(1 hunks)app/client/src/utils/hooks/useFeatureFlagOverride.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- app/client/src/components/editorComponents/Debugger/constants.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/index.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/LoadingOverlay.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/EmptyVisualization.tsx
- app/client/src/utils/hooks/useFeatureFlagOverride.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/SuggestionButtons.tsx
- app/client/src/layoutSystems/anvil/integrations/selectors.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/PromptInput.tsx
- app/client/src/entities/Action/index.ts
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useSaveVisualization.ts
- app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx
[error] 94-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (6)
app/client/src/ce/entities/FeatureFlag.ts (2)
57-57: New feature flag added correctlyThe addition of the
release_fn_calling_enabledfeature flag follows the project's naming convention pattern and is consistent with other release flags in the file.
103-103: Default value set appropriatelySetting the default value to
falseis appropriate for a new feature flag, allowing for controlled rollout.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/Visualization.tsx (1)
99-108: Good use of ErrorBoundary for visualization errors.The ErrorBoundary implementation provides a graceful fallback when visualizations fail to render, which is a robust approach for error handling.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/components/Result.tsx (2)
36-87: Robust handling of inter-iframe communication.The switch cases neatly handle event types. However, if new event types are added, consider extracting the logic into a separate function or dictionary-based handler to keep this effect concise and scalable.
133-133:Details
❓ Verification inconclusive
Verify sandbox permissions for security.
The current sandbox permissions (
allow-scripts allow-same-origin) are necessary for functionality but ensure they're not overly permissive for security. Consider ifallow-same-originis strictly required or if more restricted communication methods could be used.
🏁 Script executed:
#!/bin/bash # Check for recommended security practices for sandboxed iframes rg -A 10 -B 10 "sandbox=" --glob "*.tsx" --glob "*.jsx"Length of output: 10970
Security Sandbox Permissions Review
Please verify that using both
allow-scriptsandallow-same-originin this iframe is truly necessary. Whileallow-scriptsis required to execute embedded scripts, theallow-same-originpermission may introduce additional security risks if it's not essential for the widget’s functionality. Notably, similar components (e.g., in ExternalWidget) use a more restrictive setting. Double-check if the plugin visualization really needs same-origin access or if it can be safely omitted or replaced with a less permissive option.app/client/src/PluginActionEditor/components/PluginActionResponse/components/Visualization/useGenerateVisualization.ts (1)
67-76: Well-structured return value with clear navigation state.The returned object provides all necessary values and functions with clear boolean flags for navigation state, making it easy to use in the consuming component.
|
Deploy-Preview-URL: https://ce-39690.dp.appsmith.com |
## Description Fixes appsmithorg#39554 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13811044062> > Commit: 1a5b458 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13811044062&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 12 Mar 2025 12:57:34 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Added a new forward arrow icon for enhanced design consistency. - Expanded sidebar functionality to support an optional extra title button for additional actions. - Introduced a comprehensive visualization experience, including interactive components for generating, saving, and displaying visualizations, along with prompt inputs, suggestions, and a results view. - Enhanced action capabilities to support visualization data and debugging with a new visualization tab. - Enabled new release functionalities via an updated feature flag system. - **Style** - Refined sidebar title spacing and layout for improved presentation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Fixes #39554
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13811044062
Commit: 1a5b458
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 12 Mar 2025 12:57:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style