Closed
Conversation
|
📊 Coverage check completed. See workflow run for details. |
🤖 AI PR Validation ReportPR Review ResultsThank you for your submission! Here's detailed feedback on your PR title and body compliance:✅ PR Title
✅ Commit Type
✅ Risk Level
✅ What & Why
✅ Impact of Change
✅ Test Plan
✅ Contributors
|
| Section | Status | Recommendation |
|---|---|---|
| Title | ✅ | No change needed. |
| Commit Type | ✅ | Correct (refactor). |
| Risk Level | ✅ | risk:medium appropriate. |
| What & Why | ✅ | Clear; consider a short migration note. |
| Impact of Change | ✅ | Good; add example usage for new hooks if helpful. |
| Test Plan | ✅ | Unit tests present in diff; consider E2E if needed. |
| Contributors | ✅ | OK. |
| Screenshots/Videos | N/A is fine; add visuals only if UI behavior changed. |
Final Message
This PR body and title largely follow the required template and are high quality. The risk label matches the code changes and the diff shows unit tests updated to reflect the refactor. A few small suggestions to make review and future maintenance easier:
- Add a very short "Migration / Developer guidance" subsection (1–3 examples) in the PR body showing old → new usage for the most common external hooks/actions that changed (e.g.,
resetDesignerViewdispatch →useResetDesignerView()usage,setNodeDescription→updateOperationDescription({ id, description })). This helps consumers update call sites quickly. - If there are known edge-case behaviors (undo/redo, modal promise resolution), either add targeted unit tests or call them out explicitly in Test Plan with rationale for manual testing.
- Optionally expand the PR title with a parenthetical list of the biggest changes for quick scanning, though the current title is acceptable.
Please update only if you want to add migration examples or explicitly call out any remaining manual test coverage; otherwise this PR passes the PR title/body checklist. Thank you for the thorough description and for updating tests along with the refactor!
Last updated: Tue, 07 Apr 2026 19:04:49 GMT
github-actions
bot
added
pr-validated
needs-pr-update
and removed
pr-validated
needs-pr-update
labels
Contributor
Author
|
Closing this, will be going with a different approach. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors designer-v2 state management by removing several Redux slices (designerView, modal, staticResultsSchema) and consolidating state into React contexts and the existing operationMetadata slice, while simplifying panel selection state and reducing undo/redo snapshot size.
Changes:
- Moved designer view + modal state from Redux slices into React context providers (
DesignerViewContext,ModalContext), updating UI call sites and tests. - Folded static result schema/properties into
operationMetadataSliceand updated selectors, serializer/deserializer, and copy/paste paths. - Simplified panel selection to a single top-level
panel.selectedNodeIdsand excludedpanelfrom undo/redo compression.
Reviewed changes
Copilot reviewed 59 out of 59 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/usePanelTabs.tsx | Switches schema hooks to operation selectors after static results consolidation. |
| libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tabs/testingTab/index.tsx | Repoints static-result actions/selectors to operation metadata/state. |
| libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/nodeDetailsPanel.tsx | Uses DesignerViewContext + operationMetadata description update action. |
| libs/designer-v2/src/lib/ui/panel/nodeDetailsPanel/tests/usePanelTabs.spec.tsx | Updates mocks for moved schema selector. |
| libs/designer-v2/src/lib/ui/dialog/triggerDescriptionDialog.tsx | Migrates modal open/close to ModalContext and description to operation metadata. |
| libs/designer-v2/src/lib/ui/dialog/combineVariableDialog.tsx | Migrates combine-variable modal close/resolve to ModalContext. |
| libs/designer-v2/src/lib/ui/connections/handoffEdge.tsx | Moves edge context-menu state to DesignerViewContext. |
| libs/designer-v2/src/lib/ui/connections/dropzone.tsx | Moves edge context-menu state to DesignerViewContext. |
| libs/designer-v2/src/lib/ui/common/KindChangeDialog/test/KindChangeDialog.spec.tsx | Updates test to ModalContext close hook. |
| libs/designer-v2/src/lib/ui/common/KindChangeDialog/KindChangeDialog.tsx | Uses ModalContext instead of Redux modal slice. |
| libs/designer-v2/src/lib/ui/common/DesignerContextualMenu/DesignerContextualMenu.tsx | Uses DesignerViewContext for delete modal state. |
| libs/designer-v2/src/lib/ui/common/DeleteModal/DeleteModal.tsx | Uses DesignerViewContext to dismiss delete modal. |
| libs/designer-v2/src/lib/ui/DesignerReactFlow.tsx | Moves node context-menu state to DesignerViewContext. |
| libs/designer-v2/src/lib/ui/CustomNodes/test/SubgraphCardNode.spec.tsx | Updates mocks for DesignerViewContext hooks. |
| libs/designer-v2/src/lib/ui/CustomNodes/test/ScopeCardNode.spec.tsx | Updates mocks for DesignerViewContext hooks. |
| libs/designer-v2/src/lib/ui/CustomNodes/test/OperationCardNode.spec.tsx | Updates mocks for DesignerViewContext hooks. |
| libs/designer-v2/src/lib/ui/CustomNodes/SubgraphCardNode.tsx | Uses DesignerViewContext hooks for context menu + delete modal. |
| libs/designer-v2/src/lib/ui/CustomNodes/ScopeCardNode.tsx | Uses DesignerViewContext hooks for context menu + delete modal. |
| libs/designer-v2/src/lib/ui/CustomNodes/OperationCardNode.tsx | Uses DesignerViewContext hooks for context menu + delete modal. |
| libs/designer-v2/src/lib/ui/CustomNodes/CollapsedCardNode.tsx | Uses DesignerViewContext hook for node context menu. |
| libs/designer-v2/src/lib/ui/Controls.tsx | Uses DesignerViewContext hooks for minimap toggling. |
| libs/designer-v2/src/lib/core/utils/undoredo.ts | Excludes panel/staticResults from compressed state and updates selected node tracking. |
| libs/designer-v2/src/lib/core/utils/swagger/operation.ts | Moves addResultSchema and description initialization into operation metadata. |
| libs/designer-v2/src/lib/core/utils/test/undoredo.spec.ts | Updates tests for new panel selectedNodeIds shape and excluded slices. |
| libs/designer-v2/src/lib/core/store.ts | Removes designerView/staticResults/modal reducers from store. |
| libs/designer-v2/src/lib/core/state/workflow/workflowSlice.ts | Drops setNodeDescription reducer and reuses operationMetadata updateOperationDescription. |
| libs/designer-v2/src/lib/core/state/workflow/workflowSelectors.ts | Sources node description from operations.operationMetadata. |
| libs/designer-v2/src/lib/core/state/undoRedo/undoRedoTypes.ts | Removes panel/staticResults from UndoRedoPartialRootState. |
| libs/designer-v2/src/lib/core/state/staticresultschema/staticresultsSlice.ts | Deletes static results schema slice (moved into operation metadata). |
| libs/designer-v2/src/lib/core/state/staticresultschema/staitcresultsSelector.ts | Deletes static-results selectors (replaced by operation selector hooks). |
| libs/designer-v2/src/lib/core/state/panel/panelTypes.ts | Hoists selectedNodeIds to top-level panel state. |
| libs/designer-v2/src/lib/core/state/panel/panelSlice.ts | Consolidates selected node tracking and removes undo/redo restoration. |
| libs/designer-v2/src/lib/core/state/panel/panelSelectors.ts | Repoints selectors to new top-level selectedNodeIds. |
| libs/designer-v2/src/lib/core/state/operation/operationSelector.ts | Adds static-results schema/property hooks under operation state. |
| libs/designer-v2/src/lib/core/state/operation/operationMetadataSlice.ts | Adds staticResultSchemas/staticResultProperties and related reducers; widens description payload. |
| libs/designer-v2/src/lib/core/state/modal/modalSlice.ts | Deletes Redux modal slice (moved to ModalContext). |
| libs/designer-v2/src/lib/core/state/modal/modalSelectors.ts | Removes selectors that read from deleted modal slice. |
| libs/designer-v2/src/lib/core/state/modal/ModalContext.tsx | Introduces ModalContext + module-level service bridge for thunks. |
| libs/designer-v2/src/lib/core/state/designerView/designerViewSlice.ts | Deletes Redux designerView slice (moved to DesignerViewContext). |
| libs/designer-v2/src/lib/core/state/designerView/designerViewSelectors.ts | Re-exports DesignerViewContext hooks (no longer Redux selectors). |
| libs/designer-v2/src/lib/core/state/designerView/DesignerViewContext.tsx | Introduces DesignerViewContext provider + selector/action-equivalent hooks. |
| libs/designer-v2/src/lib/core/state/test/staticresultsSlice.spec.ts | Updates test to validate undo/redo restore via operation metadata slice. |
| libs/designer-v2/src/lib/core/state/test/panelSlice.spec.ts | Removes undo/redo restore test for panel (no longer in snapshots). |
| libs/designer-v2/src/lib/core/state/test/modalSlice.spec.ts | Replaces modal slice tests with ModalContext tests. |
| libs/designer-v2/src/lib/core/state/test/designerViewSlice.spec.ts | Replaces reducer tests with DesignerViewContext tests. |
| libs/designer-v2/src/lib/core/parsers/test/combineSequentialInitializeVariables.spec.ts | Formatting-only update in test data. |
| libs/designer-v2/src/lib/core/parsers/ParseReduxAction.ts | Uses ModalContext service to await combine-variable decision; moves initStaticResultProperties import. |
| libs/designer-v2/src/lib/core/index.ts | Re-exports DesignerViewContext reset hook + ModalContext exports; stops exporting modal slice. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/serializer.ts | Serializes description/staticResults from operation metadata state instead of removed slices. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/operationdeserializer.ts | Initializes operation description and moves addResultSchema import. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/delete.ts | Moves deinitializeStaticResultProperty import under operation metadata slice. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/copypaste.ts | Reads/writes description + static results via operation metadata/state. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/agent.ts | Moves addResultSchema import under operation metadata slice. |
| libs/designer-v2/src/lib/core/actions/bjsworkflow/add.ts | Uses ModalContext service to open kind-change dialog (no Redux modal slice). |
| libs/designer-v2/src/lib/core/DesignerProvider.tsx | Adds DesignerViewProvider + ModalProvider; resets context state on id changes. |
| libs/designer-v2/src/lib/test/redux-test-helper.tsx | Removes deleted reducers from test store. |
| libs/designer-v2/src/lib/test/mock-root-state.tsx | Removes deleted slices from mocked RootState/UndoRedoPartialRootState. |
| apps/vs-code-react/src/app/designer/DesignerCommandBar/indexV2.tsx | Switches from dispatching resetDesignerView to using context reset hook. |
| apps/Standalone/src/designer/app/AzureLogicAppsDesigner/DesignerCommandBarV2.tsx | Switches from dispatching resetDesignerView to using context reset hook. |
Comments suppressed due to low confidence (1)
libs/designer-v2/src/lib/core/state/operation/operationSelector.ts:1
- The reducer callback mutates the
accparameter, but theno-param-reassignsuppression was removed. Ifno-param-reassignis enforced, this will fail lint/CI. Either restore the targeted ESLint suppression for this block or rewrite thereduceto return a new accumulator object without mutating the parameter.
import type { NodeStaticResults } from '../../actions/bjsworkflow/staticresults';
Comment on lines
+6
to
+7
| showMinimap: boolean | undefined; | ||
| clampPan: boolean | undefined; |
There was a problem hiding this comment.
showMinimap and clampPan are now typed as boolean | undefined, which is a type regression from the previous selector behavior (always boolean). Since these values are always initialized in the provider, make them plain boolean to preserve the prior contract and avoid downstream consumers needing unnecessary undefined handling.
Suggested change
| showMinimap: boolean | undefined; | |
| clampPan: boolean | undefined; | |
| showMinimap: boolean; | |
| clampPan: boolean; |
Comment on lines
+17
to
+29
| // Module-level service holder for thunks that can't use React hooks | ||
| export interface ModalService { | ||
| openCombineVariable: () => Promise<boolean>; | ||
| openKindChange: (type: string) => void; | ||
| } | ||
|
|
||
| let _modalService: ModalService | null = null; | ||
| export const getModalService = (): ModalService => { | ||
| if (!_modalService) { | ||
| throw new Error('ModalService not initialized. Ensure ModalProvider is mounted.'); | ||
| } | ||
| return _modalService; | ||
| }; |
There was a problem hiding this comment.
Using a module-level singleton service has two concrete pitfalls: (1) it’s not initialized until the first useEffect runs, so getModalService() can throw even after the provider renders (before effects flush), and (2) it implicitly supports only one mounted ModalProvider at a time (last mounted wins), which can break multi-designer scenarios. Consider initializing the service synchronously (e.g., during render via a stable ref / useMemo or useLayoutEffect) and/or avoiding a global singleton by injecting a modal service through thunk extraArgument or a host-provided service container.
Comment on lines
+68
to
+74
| // Set up the module-level service for thunks | ||
| useEffect(() => { | ||
| _modalService = { openCombineVariable, openKindChange }; | ||
| return () => { | ||
| _modalService = null; | ||
| }; | ||
| }, [openCombineVariable, openKindChange]); |
There was a problem hiding this comment.
Using a module-level singleton service has two concrete pitfalls: (1) it’s not initialized until the first useEffect runs, so getModalService() can throw even after the provider renders (before effects flush), and (2) it implicitly supports only one mounted ModalProvider at a time (last mounted wins), which can break multi-designer scenarios. Consider initializing the service synchronously (e.g., during render via a stable ref / useMemo or useLayoutEffect) and/or avoiding a global singleton by injecting a modal service through thunk extraArgument or a host-provided service container.
| getModalService, | ||
| } from '../modal/ModalContext'; | ||
|
|
||
| const wrapper = ({ children }: { children: React.ReactNode }) => createElement(ModalProvider, null, children); |
There was a problem hiding this comment.
This test file references React.ReactNode but does not import the React type/namespace, which will fail TypeScript compilation in setups where React isn’t a global namespace. Import type React from 'react' (or use import('react').ReactNode) for the wrapper props type.
| export { initializeNotes } from './state/notes/notesSlice'; | ||
| export { useCanUndo, useCanRedo } from './state/undoRedo/undoRedoSelectors'; | ||
| export { resetDesignerView } from './state/designerView/designerViewSlice'; | ||
| export { useResetDesignerView } from './state/designerView/DesignerViewContext'; |
There was a problem hiding this comment.
The public API now exposes useResetDesignerView (hook) instead of a dispatchable action (resetDesignerView). This makes resetting designer-view state impossible from non-React contexts (similar to why getModalService was introduced). Consider exposing an imperative reset entrypoint (e.g., a DesignerView service getter like the modal service) or a backwards-compatible export name to reduce friction for consumers that previously called this from outside React components.
Suggested change
| export { useResetDesignerView } from './state/designerView/DesignerViewContext'; | |
| export { | |
| useResetDesignerView, | |
| useResetDesignerView as resetDesignerView, | |
| } from './state/designerView/DesignerViewContext'; |
Comment on lines
130
to
+131
| export * from './state/modal/modalSelectors'; | ||
| export * from './state/modal/modalSlice'; | ||
| export * from './state/modal/ModalContext'; |
There was a problem hiding this comment.
The public API now exposes useResetDesignerView (hook) instead of a dispatchable action (resetDesignerView). This makes resetting designer-view state impossible from non-React contexts (similar to why getModalService was introduced). Consider exposing an imperative reset entrypoint (e.g., a DesignerView service getter like the modal service) or a backwards-compatible export name to reduce friction for consumers that previously called this from outside React components.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Type
Risk Level
What & Why
Simplifies the Redux state architecture in designer-v2 by reducing slice count, eliminating duplicated state, and moving non-global/non-serializable state out of Redux. This is a 6-commit series:
Impact of Change
Test Plan
Contributors
@rllyy97
Screenshots/Videos
N/A