Manage state outside of React #1159
Conversation
philippfromme
force-pushed
the
1131-stale-element-fix
branch
from
992aedc to
6e4cd5c
Compare
philippfromme
force-pushed
the
1131-stale-element-fix
branch
from
6e4cd5c to
87c1e3f
Compare
bpmn-io-tasks
bot
added
needs review
There was a problem hiding this comment.
Pull Request Overview
This PR refactors state management by moving selected element and group state outside of the React component into the BpmnPropertiesPanelRenderer class. This addresses issue #1131 by preventing stale selected elements through direct state management rather than React hooks.
Key Changes
- Moved element selection and group calculation logic from
BpmnPropertiesPanelReact component toBpmnPropertiesPanelRenderer - Replaced React hooks (
useState,useEffect,useMemo) with direct event listener patterns in the renderer - Updated provider constructors to initialize
_injectorbefore registering with the properties panel
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/render/BpmnPropertiesPanel.js | Removed React state management hooks and event listeners; simplified to pure rendering component |
| src/render/BpmnPropertiesPanelRenderer.js | Added state management for selected element and groups; implemented event listeners for updates |
| src/provider/bpmn/BpmnPropertiesProvider.js | Reordered constructor to initialize _injector before registration |
| src/provider/camunda-platform/CamundaPlatformPropertiesProvider.js | Reordered constructor to initialize _injector before registration |
| src/provider/zeebe/ZeebePropertiesProvider.js | Reordered constructor to initialize _injector before registration |
| test/spec/BpmnPropertiesPanel.spec.js | Updated tests to pass groups directly instead of using getProviders |
| test/spec/BpmnPropertiesPanelRenderer.spec.js | Added comprehensive rendering tests for various state changes; wrapped existing test in describe block |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
philippfromme
force-pushed
the
1131-stale-element-fix
branch
from
87c1e3f to
c107e9d
Compare
| }); | ||
| }; | ||
|
|
||
| const updateSelectedElement = () => { |
There was a problem hiding this comment.
I find it a bit confusing that we define these methods inline. I will fix that.
|
To fix: label selection leads to an error 
|
| // handle labels | ||
| if (newSelectedElement && newSelectedElement.type === 'label') { | ||
| newSelectedElement = newSelectedElement.labelTarget; | ||
| } |
Fixed via 8c91763 |
|
Thanks for reviewing and fixing @barmac 🏅 Do you think the overall approach makes sense? |
| import { | ||
| useState, | ||
| useMemo, | ||
| useEffect, | ||
| useCallback | ||
| } from '@bpmn-io/properties-panel/preact/hooks'; |
There was a problem hiding this comment.
I am super happy to see this removed.
Absolutely! Great work |
|
@barmac I will go ahead and implement the same change in https://github.com/bpmn-io/dmn-js-properties-panel. |
Proposed Changes
This PR moves state like the selected element and the groups for the selected elements outside of the properties panel React component. This prevents stale selected elements like in #1131.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}