Update/collapse panels on preview #3255
Conversation
…omponent in between preview and designer states
WalkthroughThe changes introduce state management for tracking the previously selected component in the form designer. This includes new context properties, actions, reducer logic, and provider logic to save and restore the selected component when toggling between 'designer' and 'edit' modes. Additional updates include code formatting and import cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PreviewButton
participant FormDesignerProvider
participant Reducer
User->>PreviewButton: Clicks preview button
PreviewButton->>FormDesignerProvider: Toggles formMode
alt Switching to 'edit'
FormDesignerProvider->>Reducer: setPreviousSelectedComponentAction(current selected)
FormDesignerProvider->>Reducer: setSelectedComponent(null)
else Switching to 'designer'
FormDesignerProvider->>Reducer: setSelectedComponent(previous selected)
FormDesignerProvider->>Reducer: setPreviousSelectedComponent(null)
end
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shesha-reactjs/src/providers/formDesigner/reducer.ts (1)
494-505: Duplicate action handlers could be DRY-ed up
SetSelectedComponentandSetPreviousSelectedComponentshare identical logic.
A helper likeupdateSelection(state, payload, keyPrefix)or switching on anisPreviousflag would remove duplication and make future changes less error-prone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
shesha-reactjs/src/components/formDesigner/index.tsx(1 hunks)shesha-reactjs/src/components/formDesigner/toolbar/previewButton.tsx(1 hunks)shesha-reactjs/src/providers/formDesigner/actions.ts(1 hunks)shesha-reactjs/src/providers/formDesigner/contexts.tsx(1 hunks)shesha-reactjs/src/providers/formDesigner/index.tsx(1 hunks)shesha-reactjs/src/providers/formDesigner/reducer.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-attempt
🔇 Additional comments (7)
shesha-reactjs/src/components/formDesigner/index.tsx (2)
1-5: Cleaned up import statements effectively.The import statements have been organized by combining multiple React imports into a single line, which follows best practices for cleaner code.
6-8: Improved interface formatting.The interface formatting now follows consistent indentation, enhancing readability.
shesha-reactjs/src/components/formDesigner/toolbar/previewButton.tsx (1)
16-21: Preserved component selection state during mode toggle.The removal of
setSelectedComponent(null)enables preserving the selected component when toggling between 'designer' and 'edit' modes. This aligns with the PR's goal of maintaining component selection state during mode transitions.shesha-reactjs/src/providers/formDesigner/actions.ts (2)
43-43: Added new action type to support previous component selection tracking.The new
SetPreviousSelectedComponentaction type will enable tracking of the previously selected component in the form designer state, aligning with the PR's goals.
126-129: Added action creator for managing previous component selection.The
setPreviousSelectedComponentActioncreator uses the same payload type assetSelectedComponentAction, providing a consistent approach to component selection management.shesha-reactjs/src/providers/formDesigner/contexts.tsx (1)
30-31: Added state tracking for previously selected component.The new properties
previousSelectedComponentIdandpreviousSelectedComponentRefenable preserving and restoring component selection when toggling between 'designer' and 'edit' modes.shesha-reactjs/src/providers/formDesigner/index.tsx (1)
234-242: Potential type-safety issues when dispatchingnullIDs
setSelectedComponent(null)andsetPreviousSelectedComponent(null)send{ id: null }.
IfISetSelectedComponentPayload.idis typed asstring(non-nullable), this will break type-checking and risks runtime assumptions elsewhere.Please confirm the interface allows
nullor adjust the payload definition tostring | null.
| const setSelectedComponent = useCallback((componentId: string, componentRef?: MutableRefObject<any>) => { | ||
| if (componentId !== state.present.selectedComponentId || | ||
| componentRef !== state.present.selectedComponentRef) | ||
| dispatch(setSelectedComponentAction({ id: componentId, componentRef })); | ||
| componentInitialization.current = true; | ||
| }, [dispatch]); | ||
|
|
There was a problem hiding this comment.
Missing braces make componentInitialization flag always flip to true
Because the if statement lacks braces, only the dispatch call is guarded by the condition.
componentInitialization.current = true is executed on every invocation (even when the component ID / ref is unchanged).
This defeats the optimisation further down in updateComponent where you try to skip the first render.
- const setSelectedComponent = useCallback((componentId: string, componentRef?: MutableRefObject<any>) => {
- if (componentId !== state.present.selectedComponentId ||
- componentRef !== state.present.selectedComponentRef)
- dispatch(setSelectedComponentAction({ id: componentId, componentRef }));
- componentInitialization.current = true;
- }, [dispatch]);
+ const setSelectedComponent = useCallback(
+ (componentId: string, componentRef?: MutableRefObject<any>) => {
+ if (
+ componentId !== state.present.selectedComponentId ||
+ componentRef !== state.present.selectedComponentRef
+ ) {
+ dispatch(setSelectedComponentAction({ id: componentId, componentRef }));
+ componentInitialization.current = true;
+ }
+ },
+ [dispatch, state.present.selectedComponentId, state.present.selectedComponentRef]
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setSelectedComponent = useCallback((componentId: string, componentRef?: MutableRefObject<any>) => { | |
| if (componentId !== state.present.selectedComponentId || | |
| componentRef !== state.present.selectedComponentRef) | |
| dispatch(setSelectedComponentAction({ id: componentId, componentRef })); | |
| componentInitialization.current = true; | |
| }, [dispatch]); | |
| const setSelectedComponent = useCallback( | |
| (componentId: string, componentRef?: MutableRefObject<any>) => { | |
| if ( | |
| componentId !== state.present.selectedComponentId || | |
| componentRef !== state.present.selectedComponentRef | |
| ) { | |
| dispatch(setSelectedComponentAction({ id: componentId, componentRef })); | |
| componentInitialization.current = true; | |
| } | |
| }, | |
| [dispatch, state.present.selectedComponentId, state.present.selectedComponentRef] | |
| ); |
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/formDesigner/index.tsx around lines 223 to 229,
the if statement lacks braces, causing componentInitialization.current to be set
to true on every call regardless of the condition. To fix this, add braces
around the if block so both the dispatch call and the
componentInitialization.current assignment only execute when the componentId or
componentRef changes.
| dispatch(setPreviousSelectedComponentAction({ id: componentId, componentRef })); | ||
| }, [dispatch]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Hi @teboho. There is no need to create a separate action to maintain previous selection. Current code should be a part of reducer, otherwise it causes 2 unneeded re-renders and makes the logic more complex. You just need to save and restore selection when the reducer handles SetFormMode action.
#2481
Summary by CodeRabbit