feat: Inspect State CTA for discovery #39100
Conversation
hetunandu
requested review from
ankitakinger and
alex-golovanov
and removed request for
a team
WalkthroughThe changes introduce a set of new state inspection functionalities across the UI. A new constant for "Inspect state" is defined and used across several components. New React components (header button, menu item, toolbar button) assist in dispatching debugging actions. The modifications update context menus, Peek Overlay, property panes, and debugger hooks to integrate these state inspection options. Additionally, the debugger configuration logic is refactored using a new hook for a streamlined approach. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as InspectState Component
participant R as Redux Store
participant D as Debugger
U->>C: Click "Inspect State" (button/menu)
C->>R: Dispatch setDebuggerStateInspectorSelectedItem(entityId)
C->>R: Dispatch action to open state inspector with STATE_TAB
R->>D: Update debugger configuration
sequenceDiagram
participant E as Editor
participant H as useDebuggerConfig Hook
participant L as useLocation
participant S as Redux Store
E->>H: Invoke useDebuggerConfig()
H->>L: Retrieve current route and focus info
H->>S: Select IDE view mode
H-->>E: Return debugger configuration based on focus and view mode
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
|
# Conflicts: # app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.tsx
|
/build-deploy-preview |
github-actions
bot
added
Debugger Product
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13191905844. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx (1)
18-23: Consider extracting handleSelect logic to a shared hookThe handleSelect logic is duplicated across InspectStateMenuItem and InspectStateHeaderButton.
Create a new hook:
function useInspectState(entityId: string) { const dispatch = useDispatch(); const config = useDebuggerConfig(); return useCallback(() => { dispatch(setDebuggerStateInspectorSelectedItem(entityId)); dispatch( config.set({ open: true, selectedTab: DEBUGGER_TAB_KEYS.STATE_TAB }), ); }, [config, dispatch, entityId]); }app/client/src/components/editorComponents/Debugger/hooks/useDebuggerConfig.ts (1)
23-28: Consider making the Config interface more specific.The
CanvasDebuggerStatetype is used for both the payload and return type, which might not accurately represent the state types for all configurations.Consider using generics to make the interface more flexible:
-interface Config { +interface Config<T = CanvasDebuggerState> { set: ( - payload: Partial<CanvasDebuggerState>, + payload: Partial<T>, - ) => ReduxAction<Partial<CanvasDebuggerState>>; + ) => ReduxAction<Partial<T>>; - get: (state: AppState) => CanvasDebuggerState; + get: (state: AppState) => T; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx(3 hunks)app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/styles.ts(2 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/index.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useGetDisplayData.ts(1 hunks)app/client/src/components/editorComponents/Debugger/hooks/useDebuggerConfig.ts(1 hunks)app/client/src/components/editorComponents/Debugger/hooks/useDebuggerTriggerClick.ts(1 hunks)app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/AppJSContextMenuItems.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/AppQueryContextMenuItems.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/UI/UIEntityListTree/WidgetContextMenu.tsx(3 hunks)app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx(2 hunks)app/client/src/pages/Editor/PropertyPane/PropertyPaneTitle.tsx(2 hunks)app/client/src/pages/Editor/PropertyPane/PropertyPaneView.tsx(4 hunks)app/client/src/sagas/DebuggerSagas.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/index.ts
- app/client/src/sagas/DebuggerSagas.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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
🔇 Additional comments (19)
app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useGetDisplayData.ts (1)
13-21: Verify if configTree is still required in filterInternalPropertiesThe condition only checks dataTree, but configTree is still passed to filterInternalProperties. This seems inconsistent.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usages of filterInternalProperties to verify if configTree is required rg -A 5 "filterInternalProperties"Length of output: 6019
Final Determination: The configTree parameter remains necessary.
After reviewing the search results, it's clear that:
- The filterInternalProperties function is defined to require configTree.
- Every usage (including tests and related components) continues to pass configTree.
- Even though the hook condition only checks dataTree for the property, the filter logic internally relies on configTree for its processing.
No changes are necessary regarding the use of configTree.
app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx (1)
1-30: LGTM! Well-structured component with proper optimization.The component follows React best practices with proper use of hooks and memoization.
app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx (2)
18-23: Same handleSelect logic duplicated hereThis is the third occurrence of this logic, strongly suggesting the need for a shared hook.
26-34: LGTM! Good use of Tooltip for better UX.The Tooltip implementation enhances user experience by providing clear action description.
app/client/src/components/editorComponents/Debugger/hooks/useDebuggerTriggerClick.ts (1)
1-38: Well-structured refactoring!The extraction of configuration logic into
useDebuggerConfighook improves maintainability and follows the single responsibility principle.app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/styles.ts (2)
20-27: Clean header styling implementation!The Header component properly utilizes the design system variables for consistent spacing and layout.
29-32: Improved readability with larger font size!The increase in font size from 10px to 12px enhances text readability.
app/client/src/pages/Editor/AppPluginActionEditor/components/ToolbarMenu/ToolbarMenu.tsx (1)
33-39: Well-organized menu structure!The InspectStateMenuItem is properly positioned with clear visual separation using MenuSeparator components.
app/client/src/pages/Editor/IDE/EditorPane/JS/EntityItem/AppJSContextMenuItems.tsx (1)
42-48: Consistent menu implementation!The InspectStateMenuItem is integrated consistently with other context menus, maintaining a uniform user experience.
app/client/src/pages/Editor/IDE/EditorPane/Query/EntityItem/AppQueryContextMenuItems.tsx (1)
18-18: LGTM! Clean integration of state inspection functionality.The menu structure is well-organized with proper separators, and the state inspection functionality is consistently integrated.
Also applies to: 43-50
app/client/src/pages/Editor/JSEditor/AppJSEditorContextMenu.tsx (1)
13-13: LGTM! Consistent implementation of state inspection.The changes maintain consistency with other context menus and follow the established pattern.
Also applies to: 38-44
app/client/src/components/editorComponents/Debugger/hooks/useDebuggerConfig.ts (1)
39-69: LGTM! Well-structured implementation with proper memoization.The hook is well-implemented with:
- Clear separation of concerns
- Proper handling of different view modes and entity types
- Efficient memoization based on relevant dependencies
app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx (2)
19-20: LGTM! New imports are properly organized.The imports for InspectStateHeaderButton and getEntityPayloadInfo are correctly placed and follow the project's import conventions.
107-114: LGTM! Clean implementation of the header section.The header section with conditional rendering of InspectStateHeaderButton is well-structured and follows React best practices.
app/client/src/pages/Editor/PropertyPane/PropertyPaneTitle.tsx (2)
35-35: LGTM! Improved type safety by replacing 'any'.Changed tooltipContent type from any to string, improving type safety.
208-212: Verify uniqueness of tooltipContent used as key.Using tooltipContent as key is better than using index, but we need to ensure tooltipContent values are unique to avoid React key conflicts.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for tooltipContent assignments to check for potential duplicates rg "tooltipContent.*:"Length of output: 66459
Key Uniqueness Verification in Tooltip Usage
Based on our search results, tooltipContent is declared across several components and—for example—in PropertyPaneView.tsx literal assignments like "Copy widget" and "Delete widget" appear. In PropertyPaneTitle.tsx, tooltipContent is defined as a string and then used as the key in the map. There is no evidence from our grep results of duplicate literal values being used in contexts where uniqueness is expected.
That said, the uniqueness of tooltipContent isn’t programmatically enforced; it depends entirely on the data passed through props.actions at runtime. If there's any chance that two action objects could have the same tooltip content, you could face key conflicts. You may want to consider either enforcing uniqueness upstream or using an inherently unique identifier if duplicates become possible.
app/client/src/pages/Editor/PropertyPane/PropertyPaneView.tsx (2)
220-228: LGTM! Well-structured addition of state inspection action.The new state inspection action is properly integrated into the actions array with correct type definitions.
253-253: LGTM! Proper dependency array update.The useMemo dependency array correctly includes widgetProperties?.widgetId for the actions calculation.
app/client/src/ce/constants/messages.ts (1)
1778-1778: LGTM! Consistent implementation of new message constant.The CONTEXT_INSPECT_STATE constant follows the established pattern of function-returning-string used throughout the file.
| <MenuSeparator /> | ||
| <MenuItem onClick={showBinding} startIcon="binding-new"> | ||
| {createMessage(CONTEXT_SHOW_BINDING)} | ||
| </MenuItem> |
There was a problem hiding this comment.
Remove duplicate "Show Binding" menu item.
The menu item for showing bindings appears twice, which seems unintentional.
Remove the duplicate menu item:
<MenuItem onClick={showBinding} startIcon="binding-new">
{createMessage(CONTEXT_SHOW_BINDING)}
</MenuItem>
<MenuSeparator />
- <MenuItem onClick={showBinding} startIcon="binding-new">
- {createMessage(CONTEXT_SHOW_BINDING)}
- </MenuItem>
<InspectStateMenuItem entityId={widgetId} />Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I agree. @hetunandu We will have to remove the one above the MenuSeparator.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The PR looks GTM, except for this duplicate item.
app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx
Outdated
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-39100.dp.appsmith.com |
# Conflicts: # app/client/src/components/editorComponents/Debugger/hooks/useDebuggerTriggerClick.ts # app/client/src/sagas/DebuggerSagas.ts
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/components/editorComponents/Debugger/utils/getDebuggerPaneConfig.ts (1)
43-46: Consider using a predefined constant for JS_OBJECT configuration.The current implementation creates a new object on every function call. Consider extracting it to a constant like the other configurations for better performance and consistency.
+const jsPaneDebuggerConfig: Config = { + set: setJsPaneDebuggerState, + get: getJsPaneDebuggerState, +}; + export const getDebuggerPaneConfig = ( focusInfo: FocusEntityInfo, ideViewMode: EditorViewMode, ): Config => { if (ideViewMode === EditorViewMode.SplitScreen) { return canvasDebuggerConfig; } switch (focusInfo.entity) { case FocusEntity.QUERY: return pluginActionEditorDebuggerConfig; case FocusEntity.JS_OBJECT: - return { - set: setJsPaneDebuggerState, - get: getJsPaneDebuggerState, - }; + return jsPaneDebuggerConfig; default: return canvasDebuggerConfig; } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/components/editorComponents/Debugger/hooks/useDebuggerConfig.ts(1 hunks)app/client/src/components/editorComponents/Debugger/utils/getDebuggerPaneConfig.ts(1 hunks)app/client/src/sagas/DebuggerSagas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/components/editorComponents/Debugger/hooks/useDebuggerConfig.ts
- app/client/src/ce/constants/messages.ts
- app/client/src/sagas/DebuggerSagas.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (3)
app/client/src/components/editorComponents/Debugger/utils/getDebuggerPaneConfig.ts (3)
1-14: LGTM! Imports are well-organized and properly utilized.All necessary dependencies are imported and logically grouped.
15-20: LGTM! Well-defined TypeScript interface.The Config interface is clear, properly typed, and follows TypeScript best practices.
22-29: LGTM! Clean implementation of predefined configurations.The configurations are well-structured and maintain good separation of concerns.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/selectors/debuggerSelectors.tsx (2)
106-123: Consider using enum or constants for widget types.The switch statement uses hardcoded strings for widget types. This could lead to maintenance issues.
+const WIDGET_TYPES = { + TABS: "TABS_WIDGET", + MODAL: "MODAL_WIDGET", +} as const; switch (parentWidgetData.type) { - case "TABS_WIDGET": + case WIDGET_TYPES.TABS: const isTabContentVisible = !!parentWidgetData.isVisible && parentWidgetData.selectedTab === currentWidgetData.tabName; return isTabContentVisible ? isParentVisible(parentWidgetData, canvasWidgets, dataTree) : false; - case "MODAL_WIDGET": + case WIDGET_TYPES.MODAL: return !!parentWidgetData.isVisible;🧰 Tools
🪛 Biome (1.9.4)
[error] 110-112: 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)
110-112: Add type annotations for selectedTab and tabName.As noted in the comment, these properties lack type annotations which could lead to type-safety issues.
+interface TabWidgetData extends WidgetEntity { + selectedTab: string; +} +interface TabContentWidgetData extends WidgetEntity { + tabName: string; +} -const isTabContentVisible = +const isTabContentVisible = ( + parentWidgetData: TabWidgetData, + currentWidgetData: TabContentWidgetData +) => !!parentWidgetData.isVisible && parentWidgetData.selectedTab === currentWidgetData.tabName;🧰 Tools
🪛 Biome (1.9.4)
[error] 110-112: 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/actions/debuggerActions.ts(0 hunks)app/client/src/actions/debuggerStateInspector.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorState.ts(1 hunks)app/client/src/sagas/DebuggerSagas.ts(1 hunks)app/client/src/selectors/debuggerSelectors.tsx(1 hunks)app/client/src/selectors/debuggerStateInspector.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/actions/debuggerActions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/client/src/sagas/DebuggerSagas.ts
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/src/selectors/debuggerStateInspector.ts (1)
1-4: LGTM! Clean and type-safe selector implementation.The selector follows Redux best practices and provides type safety through the AppState type.
app/client/src/actions/debuggerStateInspector.ts (1)
1-8: LGTM! Well-structured action creator.The action creator follows Redux patterns and maintains type safety.
app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorState.ts (1)
1-18: LGTM! Well-implemented custom hook.The hook follows React patterns and provides a clean API for state management.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
|
I am reducing the cycle deps in another PR in another area of the codebase. I am not able to avoid adding new ones in this change |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (1)
1574-1599: Consider adding TypeScript type safety for iconName prop.The
IconProvidercomponent acceptsiconNameas a string, but we have a typeIconNamesthat could be used to provide better type safety.Consider updating the props type:
-export function IconProvider(props: { - iconName: string; +export function IconProvider(props: { + iconName: IconNames; size?: string; color?: string; })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/packages/design-system/ads/src/__assets__/icons/ads/state-inspector.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx(2 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateMenuItem.tsx
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateHeaderButton.tsx
- app/client/src/components/editorComponents/Debugger/StateInspector/CTAs/InspectStateToolbarButton.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (2)
app/client/packages/design-system/ads/src/Icon/Icon.provider.tsx (2)
1126-1128: LGTM!The
StateInspectorIconimport follows the established pattern and conventions.
1567-1567: LGTM!The new entry in
ICON_LOOKUPis properly placed and follows the naming convention.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
…ithorg/appsmith into feat/state-inspector-context-menu
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
## Description Remove specific cycle dependencies created because a file was both importing and exporting items it does not modify. By reducing cycle dependencies here, I am buying credits that allow adding cycle dependencies in another PR but very hard to avoid This PR reduces the deps by 4 EE PR: appsmithorg/appsmith-ee#6214 PR which adds the cycle deps: #39100 ## 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/13324385763> > Commit: 34a0f45 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13324385763&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 14 Feb 2025 07:54:00 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 - **New Features** - Introduced a “Show More” option in the code editor’s command palette for additional suggestions. - Enabled default expansion for table widget panels to improve content visibility. - **Refactor** - Consolidated entity and type management for a more consistent and maintainable platform. - **Improvements** - Enhanced widget behavior with dynamic updates in dropdowns, file pickers, and map styling for a smoother user experience. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
hetunandu
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
app/client/cypress/support/Pages/PeekOverlay.ts (2)
42-42:⚠️ Potential issueRemove usage of agHelper.Sleep().
The use of
agHelper.Sleep()is against coding guidelines. Consider using Cypress's built-in retry-ability and assertions instead.Replace the Sleep() calls with proper Cypress commands:
- this.agHelper.Sleep(); + this.agHelper.GetElement(this.locators._overlayContainer).should('be.visible');Also applies to: 49-49
15-16: 🛠️ Refactor suggestionReplace string-based selector with data-testid.
Using string-based selectors like
:contains()is not recommended. Consider using data-testid attributes instead.- _fileOperation: (operation: string) => - `.t--file-operation:contains("${operation}")`, + _fileOperation: (operation: string) => + `[data-testid="t--file-operation-${operation}"]`,
♻️ Duplicate comments (1)
app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx (1)
59-66:⚠️ Potential issueAdd error handling and type safety checks.
The current implementation has potential runtime issues:
- No validation for invalid entity types
- No type checking for entityInfo
- Silent failure if configTree[objectName] is undefined
Consider adding proper error handling:
let id: string | undefined; const entityType = get(dataTree, [objectName, "ENTITY_TYPE"]); - if (entityType && objectName in configTree) { + if (entityType && + objectName in configTree && + entityType in getEntityPayloadInfo) { const entityInfo = getEntityPayloadInfo[entityType](configTree[objectName]); - if (entityInfo) id = entityInfo.id; + if (entityInfo && 'id' in entityInfo) { + id = entityInfo.id; + } }
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/PeekOverlay/PeekOverlay_Spec.ts (1)
16-17: Avoid hardcoded coordinates in widget placement.Using hardcoded coordinates (500, 100) for widget placement can make tests brittle. Consider using relative positioning or data-* attributes.
- entityExplorer.DragDropWidgetNVerify("tablewidgetv2", 500, 100); + entityExplorer.DragDropWidgetNVerify("tablewidgetv2", { + dropZoneSelector: "[data-testid=main-container]", + position: "center" + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/PeekOverlay/PeekOverlay_Spec.ts(1 hunks)app/client/cypress/support/Pages/PeekOverlay.ts(2 hunks)app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/PeekOverlay.tsapp/client/cypress/e2e/Regression/ClientSide/PeekOverlay/PeekOverlay_Spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- 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 (4)
app/client/cypress/support/Pages/PeekOverlay.ts (2)
7-7: LGTM! Good use of data-testid attribute.The new locator follows best practices by using a data-testid attribute for test selection.
111-114: LGTM! Clean implementation of VerifyDataType method.The method correctly uses the new data-testid locator to verify the data type text.
app/client/src/components/editorComponents/CodeEditor/PeekOverlayPopup/PeekOverlayPopup.tsx (2)
9-9: LGTM! Import statements are well organized.The new imports are correctly placed and organized with the rest of the imports.
Also applies to: 19-20
112-122: LGTM! UI changes are well structured.The Header component implementation is clean with:
- Proper test IDs for testing
- Clear conditional rendering logic
- Semantic HTML structure
|
|
||
| // check basic object | ||
| peekOverlay.HoverCode(9, 3, "objectData"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("object"); | ||
| peekOverlay.CheckBasicObjectInOverlay({ x: 123, y: "123" }); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // check null - with this keyword | ||
| peekOverlay.HoverCode(10, 3, "nullData"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("null"); | ||
| peekOverlay.CheckPrimitiveValue("null"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // check number | ||
| peekOverlay.HoverCode(11, 3, "numberData"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("number"); | ||
| peekOverlay.CheckPrimitiveValue("1"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // check boolean | ||
| peekOverlay.HoverCode(12, 3, "isLoading"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("boolean"); | ||
| peekOverlay.CheckPrimitiveValue("false"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // TODO: handle this function failure on CI tests -> "function(){}" | ||
| // check function | ||
| // peekOverlay.HoverCode(13, 3, "run"); | ||
| // peekOverlay.IsOverlayOpen(); | ||
| // peekOverlay.VerifyDataType("function"); | ||
| // peekOverlay.CheckPrimitiveValue("function () {}"); | ||
| // peekOverlay.ResetHover(); | ||
|
|
||
| // check undefined | ||
| peekOverlay.HoverCode(14, 3, "data"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("undefined"); | ||
| peekOverlay.CheckPrimitiveValue("undefined"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // check string | ||
| peekOverlay.HoverCode(15, 3, "mode"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("string"); | ||
| peekOverlay.CheckPrimitiveValue("EDIT"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // check if overlay closes | ||
| peekOverlay.HoverCode(16, 3, "store"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.ResetHover(); | ||
| peekOverlay.IsOverlayOpen(false); | ||
|
|
||
| // widget object | ||
| peekOverlay.HoverCode(17, 1, "Table1"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("object"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // widget property | ||
| peekOverlay.HoverCode(18, 3, "pageNo"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("number"); | ||
| peekOverlay.CheckPrimitiveValue("1"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // widget property | ||
| peekOverlay.HoverCode(19, 3, "tableData"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("array"); | ||
| peekOverlay.CheckObjectArrayInOverlay([{}, {}, {}]); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // basic nested property | ||
| peekOverlay.HoverCode(20, 7, "id"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("number"); | ||
| peekOverlay.CheckPrimitiveValue("1"); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // undefined object | ||
| peekOverlay.HoverCode(21, 1, "aljshdlja"); | ||
| peekOverlay.IsOverlayOpen(false); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // this keyword | ||
| peekOverlay.HoverCode(22, 3, "numArray"); | ||
| peekOverlay.IsOverlayOpen(); | ||
| peekOverlay.VerifyDataType("array"); | ||
| peekOverlay.CheckPrimitveArrayInOverlay([1, 2, 3]); | ||
| peekOverlay.ResetHover(); | ||
|
|
||
| // pageList is an internal property - peek overlay shouldn't work | ||
| peekOverlay.HoverCode(23, 1, "pageList"); | ||
| peekOverlay.IsOverlayOpen(false); | ||
| peekOverlay.ResetHover(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor large test case into smaller, focused test cases.
The current test case contains multiple assertions testing different aspects of the peek overlay. Consider breaking it down into smaller, focused test cases for better maintainability and readability.
Suggested structure:
describe("Peek overlay", { tags: ["@tag.JS", "@tag.Binding", "@tag.IDE"] }, () => {
beforeEach(() => {
// Setup code: Create widget, API, and JS object
});
it("should display primitive data types correctly", () => {
// Test number, boolean, string, null, undefined
});
it("should display complex data types correctly", () => {
// Test arrays and objects
});
it("should display widget properties correctly", () => {
// Test widget object and properties
});
it("should handle edge cases correctly", () => {
// Test undefined objects and internal properties
});
});| // TODO: handle this function failure on CI tests -> "function(){}" | ||
| // check function | ||
| // peekOverlay.HoverCode(13, 3, "run"); | ||
| // peekOverlay.IsOverlayOpen(); | ||
| // peekOverlay.VerifyDataType("function"); | ||
| // peekOverlay.CheckPrimitiveValue("function () {}"); | ||
| // peekOverlay.ResetHover(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TODO comment for function type testing.
There's a TODO comment about handling function failure in CI tests. This should be addressed to ensure complete test coverage.
Let's verify if this is a known issue:
🏁 Script executed:
#!/bin/bash
# Search for similar TODOs or related issues
rg -i "handle.*function.*failure.*CI" .
gh issue list --search "function failure CI tests"Length of output: 130884
Action Required: Implement Function Failure Handling for CI Tests
It appears that in file
app/client/cypress/e2e/Regression/ClientSide/PeekOverlay/PeekOverlay_Spec.ts (lines 77-83) you have a pending TODO regarding function failure handling on CI tests (as noted by the comment “TODO: handle this function failure on CI tests -> "function(){}"). Please address this by either properly implementing error handling and assertions for the function type testing or updating the test to reliably reproduce and manage the failure without leaving outdated commented code. This will help ensure complete and robust test coverage on CI.
| agHelper.Sleep(2000); | ||
| apiPage.RunAPI(); | ||
| apiPage.CreateAndFillApi( | ||
| dataManager.dsValues[dataManager.defaultEnviorment].mockApiUrl, | ||
| ); | ||
| agHelper.Sleep(2000); | ||
|
|
||
| jsEditor.CreateJSObject(JsObjectContent, { | ||
| paste: true, | ||
| completeReplace: true, | ||
| toRun: false, | ||
| shouldCreateNewJSObj: true, | ||
| lineNumber: 0, | ||
| prettify: true, | ||
| }); | ||
| jsEditor.SelectFunctionDropdown("myFun2"); | ||
| jsEditor.RunJSObj(); | ||
| agHelper.Sleep(); |
There was a problem hiding this comment.
Remove usage of Sleep() calls.
According to the coding guidelines, we should avoid using agHelper.Sleep(). Instead, use Cypress's built-in waiting mechanisms.
Apply this diff to replace Sleep calls with appropriate wait conditions:
- agHelper.Sleep(2000);
+ cy.wait("@postExecute").its("response.statusCode").should("eq", 200);
apiPage.RunAPI();
apiPage.CreateAndFillApi(
dataManager.dsValues[dataManager.defaultEnviorment].mockApiUrl,
);
- agHelper.Sleep(2000);
+ cy.wait("@postExecute").its("response.statusCode").should("eq", 200);
jsEditor.CreateJSObject(JsObjectContent, {
paste: true,
completeReplace: true,
toRun: false,
shouldCreateNewJSObj: true,
lineNumber: 0,
prettify: true,
});
jsEditor.SelectFunctionDropdown("myFun2");
jsEditor.RunJSObj();
- agHelper.Sleep();
+ cy.wait("@postExecute").its("response.statusCode").should("eq", 200);📝 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.
| agHelper.Sleep(2000); | |
| apiPage.RunAPI(); | |
| apiPage.CreateAndFillApi( | |
| dataManager.dsValues[dataManager.defaultEnviorment].mockApiUrl, | |
| ); | |
| agHelper.Sleep(2000); | |
| jsEditor.CreateJSObject(JsObjectContent, { | |
| paste: true, | |
| completeReplace: true, | |
| toRun: false, | |
| shouldCreateNewJSObj: true, | |
| lineNumber: 0, | |
| prettify: true, | |
| }); | |
| jsEditor.SelectFunctionDropdown("myFun2"); | |
| jsEditor.RunJSObj(); | |
| agHelper.Sleep(); | |
| cy.wait("@postExecute").its("response.statusCode").should("eq", 200); | |
| apiPage.RunAPI(); | |
| apiPage.CreateAndFillApi( | |
| dataManager.dsValues[dataManager.defaultEnviorment].mockApiUrl, | |
| ); | |
| cy.wait("@postExecute").its("response.statusCode").should("eq", 200); | |
| jsEditor.CreateJSObject(JsObjectContent, { | |
| paste: true, | |
| completeReplace: true, | |
| toRun: false, | |
| shouldCreateNewJSObj: true, | |
| lineNumber: 0, | |
| prettify: true, | |
| }); | |
| jsEditor.SelectFunctionDropdown("myFun2"); | |
| jsEditor.RunJSObj(); | |
| cy.wait("@postExecute").its("response.statusCode").should("eq", 200); |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
Description
Adds various discovery points for State Inspector
Fixes #38934
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13329714726
Commit: e9dc239
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 14 Feb 2025 15:47:53 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
InspectStateHeaderButtonandInspectStateMenuItemfor enhanced state inspection capabilities.