fix: Selected item in State Inspector #38955
Conversation
WalkthroughThis pull request refactors how the State Inspector manages the selected item. The changes switch from using a complex Changes
Sequence Diagram(s)sequenceDiagram
participant SI as StateInspector Component
participant GD as useGetDisplayData Hook
participant RS as Redux Store
SI->>GD: Call useGetDisplayData(selectedItemName)
GD->>RS: Retrieve dataTree, configTree, jsActions
RS-->>GD: Return state data
GD-->>SI: Return computed display data
Assessment against linked issues
Suggested labels
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
|
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useGetDisplayData.ts (1)
7-25: Consider adding error boundary for robust error handling.While the hook handles missing data gracefully, consider wrapping the
filterInternalPropertiescall in a try-catch block to handle potential runtime errors.export function useGetDisplayData(selectedItemName: string) { const dataTree = useSelector(getDataTree); const configTree = useSelector(getConfigTree); const jsActions = useSelector(getJSCollections); return useMemo(() => { if (selectedItemName in dataTree && selectedItemName in configTree) { + try { return filterInternalProperties( selectedItemName, dataTree[selectedItemName], jsActions, dataTree, configTree, ); + } catch (error) { + console.error("Error filtering properties:", error); + return ""; + } } return ""; }, [configTree, dataTree, jsActions, selectedItemName]); }app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorItems.ts (1)
22-45: Consider memoizing selectedItem transformation separately.The selectedItem transformation could be memoized separately from groups to avoid unnecessary object creation when only groups change.
- const [groups, selectedItem] = useMemo(() => { + const groups = useMemo(() => { const allGroups = [queries, jsItems, uiItems, globalItems]; const processedGroups = allGroups .filter((groupedItems) => groupedItems.items.length > 0) .map((groupedItems) => ({ group: groupedItems.group, items: groupedItems.items.map((item) => enhanceItemForListItem(item, selectedItemId, setSelectedItem), ), })); + return processedGroups; + }, [globalItems, jsItems, queries, selectedItemId, setSelectedItem, uiItems]); + + const selectedItem = useMemo(() => { const selectedItemFromGroups = processedGroups .flatMap((group) => group.items) .find((item) => item.id === selectedItemId); - const selectedItem: GenericEntityItem | undefined = { + return selectedItemFromGroups ? { key: selectedItemFromGroups?.id || "", title: selectedItemFromGroups?.title || "", icon: selectedItemFromGroups?.startIcon, - }; - - return [processedGroups, selectedItem]; - }, [globalItems, jsItems, queries, selectedItemId, setSelectedItem, uiItems]); + } : undefined; + }, [groups, selectedItemId]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/src/actions/debuggerActions.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.test.tsx(7 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx(2 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/index.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useGetDisplayData.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorItems.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorState.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/utils.ts(1 hunks)app/client/src/reducers/uiReducers/debuggerReducer.ts(2 hunks)app/client/src/selectors/debuggerSelectors.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/components/editorComponents/Debugger/StateInspector/hooks/index.ts
🔇 Additional comments (11)
app/client/src/components/editorComponents/Debugger/StateInspector/utils.ts (1)
4-15: LGTM! Clean refactor to use string IDs.The changes simplify the state management by using string IDs instead of complex objects while maintaining the same functionality.
app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorState.ts (1)
5-18: LGTM! Consistent type changes.The hook's signature changes align with the refactoring to use string IDs, maintaining consistency across the codebase.
app/client/src/components/editorComponents/Debugger/StateInspector/hooks/useStateInspectorItems.ts (1)
47-56: LGTM! Clean effect implementation.The useEffect implementation for handling unselected state is clean and has proper dependencies.
app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx (1)
12-12: LGTM! Good separation of concerns.Moving the display data computation to a separate hook improves code organization and maintainability.
Also applies to: 21-22
app/client/src/actions/debuggerActions.ts (1)
151-156: LGTM! Simplified action payload.Using a string ID instead of a complex object type makes the action more maintainable and reduces unnecessary data passing.
app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.test.tsx (1)
17-18: LGTM! Comprehensive test coverage.The mock setup for the new hook is well-structured and consistent across all test cases.
Also applies to: 37-38
app/client/src/selectors/debuggerSelectors.tsx (2)
188-189: LGTM! Consistent with the new ID-based approach.The selector change maintains consistency with the broader refactoring to use string IDs.
1-1: Verify complete migration to ID-based approach.Let's ensure all related files have been updated to use the new ID-based approach.
✅ Verification successful
Migration to ID-based approach verified.
No references to the old patterns were found in the Debugger StateInspector files, confirming that the updates are complete.
- No occurrences of
GenericEntityItem.*selectedItemwere detected.- All references to
.selectedItemnow include theIdsuffix.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to GenericEntityItem in state inspector related files rg -l "GenericEntityItem.*selectedItem" "app/client/src/components/editorComponents/Debugger/StateInspector" # Search for any old selectedItem references that might have been missed rg -l "\.selectedItem(?!Id)" "app/client/src/components/editorComponents/Debugger/StateInspector"Length of output: 444
Script:
#!/bin/bash # Re-run the search commands using the --pcre2 flag to enable lookahead functionality # Search for any remaining references to GenericEntityItem using selectedItem within StateInspector files echo "Searching for 'GenericEntityItem.*selectedItem'..." rg --pcre2 -l "GenericEntityItem.*selectedItem" "app/client/src/components/editorComponents/Debugger/StateInspector" # Search for any old selectedItem references that might have been missed (i.e. not suffixed with 'Id') echo "Searching for occurrences of '.selectedItem' not followed by 'Id'..." rg --pcre2 -l "\.selectedItem(?!Id)" "app/client/src/components/editorComponents/Debugger/StateInspector"Length of output: 498
app/client/src/reducers/uiReducers/debuggerReducer.ts (3)
191-191: LGTM! Good simplification of the action payload type.The change from
GenericEntityItemtostringaligns with best practices by maintaining minimal state representation.
196-196: LGTM! Clear and consistent state property naming.The property name
selectedItemIdaccurately reflects its content and maintains consistency with the action type change.
214-214: LGTM! Type-safe interface update.The interface correctly reflects the state structure changes while maintaining nullability behavior.
Description
fixes the selected item being lost in state inspector by refactoring the logic
Fixes #38875
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13111881263
Commit: f64e20f
Cypress dashboard.
Tags:
@tag.IDESpec:
Mon, 03 Feb 2025 11:26:14 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor