refactor: Make useBpmnEditor return callback ref #14423
Conversation
📝 WalkthroughWalkthroughThe pull request involves modifications to the BPMN (Business Process Model and Notation) editor implementation across multiple files. The changes primarily focus on refactoring the Changes
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
github-actions
bot
added
area/process
TomasEng
added
skip-releasenotes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
TomasEng
added
quality/code
TomasEng
force-pushed
the
make-usebpmneditor-return-callback-ref
branch
from
d24abce to
31c7831
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14423 +/- ##
==========================================
- Coverage 95.66% 95.66% -0.01%
==========================================
Files 1880 1880
Lines 24447 24445 -2
Branches 2810 2810
==========================================
- Hits 23387 23385 -2
Misses 800 800
Partials 260 260 ☔ View full report in Codecov by Sentry. |
TomasEng
force-pushed
the
make-usebpmneditor-return-callback-ref
branch
from
31c7831 to
c31432e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/packages/process-editor/src/hooks/useBpmnEditor.ts (1)
The review suggestion is correct and should be implemented
The BpmnModelerInstance singleton requires proper cleanup when the container ref changes or becomes null. The suggested change ensures proper instance cleanup and prevents potential memory leaks from detached modeler instances.
🔗 Analysis chain
Line range hint
96-120: Verify cleanup handling for dynamic ref updates.The cleanup logic in useEffect might not handle cases where the callback ref is called multiple times with different elements.
Consider enhancing the cleanup handling:
const canvasRef = useCallback((div: HTMLDivElement | null) => { + if (div === null) { + // Cleanup when the ref is detached + destroyModeler(); + return; + } if (modelerRef.current) return; // ... rest of the initialization }, []);🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find getModeler and destroyModeler implementations ast-grep --pattern 'function getModeler($_) { $$$ }' ast-grep --pattern 'function destroyModeler($_) { $$$ }' ast-grep --pattern 'const destroyModeler = $_' # Find similar ref callback patterns ast-grep --pattern 'const $_ = useCallback((div: HTMLDivElement) => { $$$ }, $_)' # Search for modeler-related files fd modeler frontendLength of output: 5721
Script:
#!/bin/bash # Check BpmnModelerInstance implementation cat frontend/packages/process-editor/src/utils/bpmnModeler/BpmnModelerInstance.ts # Look for instance management patterns ast-grep --pattern 'class BpmnModelerInstance { $$$ }' # Search for tests that might reveal instance behavior cat frontend/packages/process-editor/src/hooks/useBpmnModeler/useBpmnModeler.test.tsLength of output: 6223
🧹 Nitpick comments (1)
frontend/packages/process-editor/src/hooks/useBpmnEditor.test.tsx (1)
154-163: Consider adding type safety to the setup function parameters.The spread operator with Parameters type provides type safety, but explicit typing would make the API more clear.
Consider this alternative:
function setup( currentEventName: string, currentEvent: any, bpmnDetails: BpmnDetails = mockBpmnDetails ): ReturnType<typeof renderUseBpmnEditor> { const div = document.createElement('div'); const hook = renderUseBpmnEditor(currentEventName, currentEvent, bpmnDetails); hook.result.current(div); return hook; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/packages/process-editor/src/components/Canvas/BPMNEditor/BPMNEditor.test.tsx(1 hunks)frontend/packages/process-editor/src/components/Canvas/BPMNEditor/BPMNEditor.tsx(1 hunks)frontend/packages/process-editor/src/hooks/useBpmnEditor.test.tsx(4 hunks)frontend/packages/process-editor/src/hooks/useBpmnEditor.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/process-editor/src/components/Canvas/BPMNEditor/BPMNEditor.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (3)
frontend/packages/process-editor/src/hooks/useBpmnEditor.ts (2)
13-13: LGTM! Type definition is clear and focused.The new type
UseBpmnEditorResultclearly represents the callback ref pattern recommended by React.
96-102: Consider adding missing dependencies to useCallback.The useCallback's empty dependency array might cause stale closure issues. The callback uses
modelerRef,getModeler, andinitializeEditorwhich should be included in the dependencies.Consider adding the dependencies and handling potential issues:
}, []); // Missing dependencies are not added to avoid getModeler to be called multiple times + }, [modelerRef, getModeler, initializeEditor]); // Add proper dependency managementIf multiple calls to getModeler are a concern, consider using a ref to track initialization:
const isInitialized = useRef(false); const canvasRef = useCallback((div: HTMLDivElement) => { if (modelerRef.current || isInitialized.current) return; isInitialized.current = true; // ... rest of the code }, [modelerRef, getModeler, initializeEditor]);frontend/packages/process-editor/src/components/Canvas/BPMNEditor/BPMNEditor.test.tsx (1)
9-9: LGTM! Mock implementation correctly matches the new callback ref pattern.The mock implementation has been properly updated to return a function, aligning with the refactored hook's return type.
Description
Made the
useBpmnEditorreturn a callback ref. This will make it easier to trigger initialization and cleanup functions.Related Issue
Verification
Summary by CodeRabbit
Refactor
useBpmnEditorhook implementation to use a callback-based approach for canvas reference managementuseBpmnEditorhookTests