Fix wec connectors #2266
Fix wec connectors #2266
Conversation
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
bot
added
the
size/XL
There was a problem hiding this comment.
Pull request overview
This PR addresses excessive edge re-rendering issues in the WEC (Web Edge Connectors) topology visualization by removing theme-dependent style updates from edges, eliminating edge caching, and adjusting the layout calculation strategy to let ReactFlow handle zoom and positioning.
Key changes:
- Removed edge caching mechanism and theme-dependent edge style updates
- Replaced zoom-based dynamic scaling with fixed layout values in
getLayoutedElements - Added CSS animations and GPU acceleration hints (will-change, transform) for smoother rendering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
frontend/src/components/wds_topology/FlowCanvas.tsx |
Added fade-in animation for edges and GPU acceleration CSS properties |
frontend/src/components/WecsTopology.tsx |
Removed edge cache, updated edge creation to use gradient styles, modified layout calculation to use fixed values, adjusted dependency arrays, and wrapped data transformation in requestAnimationFrame |
Critical Issues Found:
- Missing
nodesandedgesdependencies intransformDataToTreecallback causing stale closures - Removed deep equality check in
getLayoutedElementscould cause stale node data - Async function inside requestAnimationFrame may not execute as intended
- Ineffective memoization patterns that provide no actual benefit
- Overly aggressive caching condition that effectively disables caching
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| namespace.resourceTypes.forEach(resourceType => { | ||
| // Skip Event type resources | ||
| if (resourceType.kind.toLowerCase() === 'event') return; | ||
| requestAnimationFrame(async () => { |
There was a problem hiding this comment.
Async function inside requestAnimationFrame may not execute as intended. The requestAnimationFrame schedules a callback for the next repaint, but the async arrow function means the animation frame will resolve immediately (before the await completes), not when the data transformation is finished.
Consider restructuring to ensure the animation frame timing aligns with the actual rendering needs, or use the async/await pattern outside of requestAnimationFrame if the timing isn't critical.
| nodes.forEach(node => { | ||
| const cachedNode = nodeMap.get(node.id); | ||
| if (!cachedNode || !isEqual(cachedNode, node) || shouldRecalculate) { | ||
| if (!cachedNode || shouldRecalculate) { |
There was a problem hiding this comment.
Removed deep equality check could cause stale node data. The old logic checked !isEqual(cachedNode, node) to detect when a node's properties changed, but this check was removed. Now if a node exists in the cache and shouldRecalculate is false, it will use the cached node even if the node's data (labels, styles, etc.) has changed.
This could cause the UI to display outdated node information when node properties change without the total node count changing.
Consider restoring the !isEqual(cachedNode, node) check, or ensuring that the caching strategy accounts for node property changes.
| if (!cachedNode || shouldRecalculate) { | |
| if (!cachedNode || shouldRecalculate || !isEqual(cachedNode, node)) { |
| const memoizedNodes = useMemo(() => { | ||
| if (nodes.length === 0) return []; | ||
| return nodes; | ||
| }, [nodes]); |
There was a problem hiding this comment.
Ineffective memoization pattern. The useMemo checks for empty arrays and returns a new empty array reference each time, or returns the same nodes reference. This provides no actual memoization benefit since the dependency is [nodes], which already triggers re-computation when nodes change.
The pattern useMemo(() => condition ? [] : value, [value]) is redundant. Consider either removing this useMemo entirely or implementing meaningful memoization logic if there's a specific use case.
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
|
@kunal-511 there was re-rendering issue was also there.. fixed now?? |
Signed-off-by: kunal-511 <[email protected]>
|
@kunal-511 Can you please upload a video for preview |
kubestellar-prow
bot
added
size/L
Screencast.from.2025-12-06.18-02-26.mp4 |
Yes I think so |
|
@kunal-511 one more issue is there in binding policy can make a issue for it with your suggestion. the issue it workload and labels loding time is too much it takes 30 sec+ time. we need to optimize it with smooth loading |
|
@kunal-511 horizontal sepration should be little more in this. looks consoldiated when zoom out |
Fetching of the workloads labels and resources is a server-sent event method similar to WebSocket, so we need to handle this in different way |
yeah need to optimse backend |
Signed-off-by: kunal-511 <[email protected]>
Updated |
github-project-automation
bot
moved this from In Progress
to Ready to Merge
in KubeStellar UI Project
|
LGTM label has been added. DetailsGit tree hash: 543733214f82956dafadcad68893d823fb08c339 |
github-project-automation
bot
moved this from Ready to Merge
to Done
in KubeStellar UI Project
|
@kunal-511 Opened few issue we need to keep eye on that |
2 participants
Description
Fixes excessive edge re-rendering by removing theme-dependent style updates, eliminating edge caching, and ensuring edges are created once so ReactFlow handles layout and zoom correctly.
Related Issue
Fixes #2208
Changes Made
Checklist
Please ensure the following before submitting your PR:
Screenshots or Logs (if applicable)
Additional Notes