-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(trace-tree-node): Replacing legacy TraceTreeNode usage with new BaseNode class #101874
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
base: abdk/trace-tree-basenode-usage-v2
Are you sure you want to change the base?
feat(trace-tree-node): Replacing legacy TraceTreeNode usage with new BaseNode class #101874
Conversation
…-basenode-usage-v3
| if ( | ||
| !( | ||
| tree.transactions_count < AUTO_EXPAND_TRANSACTIONS_THRESHOLD || | ||
| tree.collapsed_nodes < AUTO_EXPAND_TRANSACTIONS_THRESHOLD || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Trace Expansion Logic Incorrect
The trace auto-expansion logic now incorrectly uses tree.collapsed_nodes instead of tree.transactions_count to determine if a trace has few transactions. This can cause traces to auto-expand when they shouldn't or fail to expand when they should, potentially impacting performance.
gggritso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
This PR removes the legacy
TraceTreeNodeclass and it's subclasses from the codebase.Also, replaces the use of
TraceTreestatics methods with the newBaseNodemethodsNote that this PR, doesn't replace the use of guards or doesn't fully use the generalizations introduced by the new
BaseNodeclass. The goal is to make the minimal amount of change to reach feature parity, pass CI and get rid of the legacyTraceTreeNodeclass.I will be following up with a stacked PR to make full use of the BaseNode class, mitigate the use of guards etc. Helps make reviewing easier/reduces scope.
At this stack all CI passes ✅, we can merge into master