fix: and router silent to avoid event change #4986
Conversation
Summary of ChangesHello @GaoFuhong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where changing edge routers during node movement could trigger unwanted events. By introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with router changes during selection dragging by making them silent to avoid firing events. The changes in src/plugin/selection/selection.ts correctly add the { silent: true } option when degrading and restoring routers, and also add a necessary asynchronous view update request.
However, I've found a bug in how edge.setRouter is called during router restoration. It doesn't handle the case where the original router is a string, leading to incorrect arguments being passed. I've provided a suggestion to fix this.
Additionally, one of the new tests in __tests__/plugin/selection.spec.ts has a logical flaw that could cause it to pass incorrectly. I've included a comment with a fix for the test logic.
Once these issues are addressed, the PR should be good to go.
| edge.removeRouter({ silent: true }) | ||
| } else { | ||
| edge.setRouter(original) | ||
| edge.setRouter(original, { silent: true }) |
There was a problem hiding this comment.
The call to edge.setRouter is incorrect when original is a string. The setRouter method has multiple overloads. When the first argument is a string, the second argument is for router args, and the third is for options (like silent). When the first argument is an object, the second is for options. Since original can be a string, { silent: true } is incorrectly passed as router arguments instead of options. You should check the type of original to call setRouter correctly.
if (typeof original === 'string') {
edge.setRouter(original, undefined, { silent: true });
} else {
edge.setRouter(original, { silent: true });
}| const restoreSilent = spySetRouter.mock.calls.some( | ||
| ([router, options]) => | ||
| (typeof router === 'string' && router === 'manhattan') || | ||
| (typeof router === 'object' && | ||
| (router as any).name === 'manhattan' && | ||
| (options as any)?.silent === true), | ||
| ) |
There was a problem hiding this comment.
The logic to check if the router is restored silently is flawed. The || operator causes short-circuiting, and the check for (options as any)?.silent === true is not always executed when router is a string. This means the test could pass even if the silent option is missing.
| const restoreSilent = spySetRouter.mock.calls.some( | |
| ([router, options]) => | |
| (typeof router === 'string' && router === 'manhattan') || | |
| (typeof router === 'object' && | |
| (router as any).name === 'manhattan' && | |
| (options as any)?.silent === true), | |
| ) | |
| const restoreSilent = spySetRouter.mock.calls.some( | |
| ([router, options]) => | |
| ((typeof router === 'string' && router === 'manhattan') || | |
| (typeof router === 'object' && | |
| (router as any).name === 'manhattan')) && | |
| (options as any)?.silent === true, | |
| ) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4986 +/- ##
=======================================
Coverage 83.21% 83.21%
=======================================
Files 334 334
Lines 38390 38396 +6
Branches 7858 7858
=======================================
+ Hits 31946 31952 +6
Misses 6421 6421
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 Description
避免在使用 movingRouterFallback 路由切换时触发事件。
🖼️ Screenshot
💡 Motivation and Context
🧩 Types of changes
🔍 Self Check before Merge