Skip to content

Fix issue with updates to server group names not triggering tree sort #8547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KijongHan
Copy link
Contributor

Resolves issue #8015

What does this PR do?

This PR resolves issue in the object explorer window for server groups, where updating the names for existing server groups don't trigger a reordering of the tree structure.

I initially tried using the inotify method with WatchEvent.Moved and WatchEvent.Changed events, passing in parameters for the updated node path for the Moved event and the parent directory "/browser" for the Changed event but neither triggers the rerender - as a workaround removing and adding the node using the existing fileTreeHandle recreates the node and places it in the appropriate sort order

@KijongHan
Copy link
Contributor Author

hi @akshay-joshi - could you please look into reapproving the workflows? rebased from upstream

@KijongHan KijongHan changed the title Fix issue with updates to server group names not triggering tree sort #8015 Fix issue with updates to server group names not triggering tree sort Mar 15, 2025
@KijongHan KijongHan changed the title #8015 Fix issue with updates to server group names not triggering tree sort Fix issue with updates to server group names not triggering tree sort Mar 15, 2025
Copy link
Contributor

@khushboovashi khushboovashi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this patch, the browser window gets blank if I update the server group name.
Please take a look at the console logs.

Screenshot 2025-03-17 at 10 34 05 AM

@KijongHan
Copy link
Contributor Author

With this patch, the browser window gets blank if I update the server group name. Please take a look at the console logs.

Screenshot 2025-03-17 at 10 34 05 AM

thanks for testing the PR @khushboovashi - which browser did you use to test the changes? While I have encountered other issues with these changes, I wasn't able to reproduce these errors with updating server groups

@KijongHan
Copy link
Contributor Author

I've encountered a few other issues testing these changes - updating any child nodes (anything under server groups) results in the nodes being placed in the root directory node (the /browser) as opposed to being under the appropriate parent (for example, updating the server name results in the node being placed under the server group directory

@khushboovashi
Copy link
Contributor

With this patch, the browser window gets blank if I update the server group name. Please take a look at the console logs.
Screenshot 2025-03-17 at 10 34 05 AM

thanks for testing the PR @khushboovashi - which browser did you use to test the changes? While I have encountered other issues with these changes, I wasn't able to reproduce these errors with updating server groups

I am using Google Chrome v 134.0

@khushboovashi
Copy link
Contributor

I've encountered a few other issues testing these changes - updating any child nodes (anything under server groups) results in the nodes being placed in the root directory node (the /browser) as opposed to being under the appropriate parent (for example, updating the server name results in the node being placed under the server group directory

I am not facing such issues. Which browser are you using? Have you tested the same thing on the released version?

@KijongHan
Copy link
Contributor Author

With this patch, the browser window gets blank if I update the server group name. Please take a look at the console logs.
Screenshot 2025-03-17 at 10 34 05 AM

thanks for testing the PR @khushboovashi - which browser did you use to test the changes? While I have encountered other issues with these changes, I wasn't able to reproduce these errors with updating server groups

I am using Google Chrome v 134.0

Thanks for those details @khushboovashi - I managed to reproduce that same issue

I've encountered a few other issues testing these changes - updating any child nodes (anything under server groups) results in the nodes being placed in the root directory node (the /browser) as opposed to being under the appropriate parent (for example, updating the server name results in the node being placed under the server group directory

I am not facing such issues. Which browser are you using? Have you tested the same thing on the released version?

yeap the release version is working as expected (although it looks like editing tables to be under different schemas is predictably keeping the table node under the previous schema, as the file tree is not being refreshed) - it was as a byproduct of the changes in this PR

@khushboovashi
Copy link
Contributor

Hi @KijongHan, are you planning to work on the reported issue, or should I close this PR?

@KijongHan
Copy link
Contributor Author

Hi @KijongHan, are you planning to work on the reported issue, or should I close this PR?

hey @khushboovashi sorry just got back from my holiday. Ill try working through the reported issue and get back to you

KijongHan added 3 commits May 3, 2025 16:36
- set RECREATE operation for server group updates
- call remove -> create => and update tree methods on the updated server group node to force resort on server group siblings
- fix issue with item.metadata.data not getting set if item.metadata was undefined
- fix issue with incorrect check on node.type (if it is a file, it should return no children, but if it is a directory it should return the node children)
@KijongHan
Copy link
Contributor Author

hi @khushboovashi - many thanks for the patience on this one. I've changed my approach for the fix on this one. Instead of trying to make the reorder work with FileTreeX viewmodel I tried calling the necessary operations to force the reorder up the call stack under browser.js so that it checks if the updated node is a server group, and if it is then it calls the RECREATE operation. it calls remove, create, then update

There are also additional changes to readNode and updateNode methods under tree_nodes.ts so that on read nodes if the type is a File then it returns an empty node children (It seems this should be the intention but let me know if otherwise) and update node takes into account an undefined node item metadata

@KijongHan KijongHan requested a review from khushboovashi May 13, 2025 13:44
@KijongHan
Copy link
Contributor Author

hi @akshay-joshi, do you think I could get some help with getting some eyes on these changes? thank you!

@khushboovashi
Copy link
Contributor

hi @akshay-joshi, do you think I could get some help with getting some eyes on these changes? thank you!

@KijongHan, I will review it.

@khushboovashi
Copy link
Contributor

Hi @KijongHan,

I tested only one scenario: update the server group, which blanks the entire screen.
I've attached a screenshot for the same.

Screenshot 2025-05-28 at 3 52 27 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants