feat(trace-tree-node): Consolidating node methods and overrides #101174
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| get id(): string { | ||
| const firstChild = this.children[0]; | ||
| return firstChild?.id; | ||
| return firstChild?.id ?? uuid4(); |
There was a problem hiding this comment.
Bug: Node ID Unstability Causes Functionality Failures
The id getters in RootNode, ParentAutogroupNode, and SiblingAutogroupNode use uuid4(). This generates a new UUID on every access, making node IDs unstable. This breaks functionality relying on consistent identifiers, such as node identification, path matching, and caching.
Additional Locations (2)
| return false; | ||
| } | ||
| return this.matchById(id); | ||
| return this.id === id; |
There was a problem hiding this comment.
Bug: Navigation Broken by ID Matching Restriction
The matchByPath method now only matches against a node's primary ID, removing the ability to find nodes by associated error or occurrence event IDs. This breaks navigation for nodes previously discoverable through those secondary IDs.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| get id(): string | undefined { | ||
| return this.value && 'event_id' in this.value ? this.value.event_id : undefined; | ||
| } | ||
|
|
||
| get op(): string | undefined { | ||
| return this.value && 'op' in this.value ? this.value.op : undefined; | ||
| } |
There was a problem hiding this comment.
Potential bug: Call to non-existent static method causes runtime TypeError
-
Description: A call to a non-existent static method
TraceTree.DirectVisibleChildrenwill cause a runtimeTypeError. The refactoring introduced an instance propertynode.directVisibleChildrenbut failed to update this specific call site. This will cause a crash when the trace waterfall attempts to display the children count for span rows. -
Suggested fix: Replace the static method call
TraceTree.DirectVisibleChildren(node).lengthwith the instance propertynode.directVisibleChildren.length.
severity: 3.0, confidence: 5.0
Did we get this right? 👍 / 👎 to inform future reviews.
| get id(): string | undefined { | ||
| return this.head.id ?? this.tail.id; | ||
| get id(): string { | ||
| return this.head.id ?? this.tail.id ?? uuid4(); |
There was a problem hiding this comment.
👀 maybe better to use uuid4() at construction time, so it's stable?
| canShowDetails = false; | ||
|
|
||
| get id(): string { | ||
| return uuid4(); |
Every node needs to consciously be assigned an
id. Helps prevent needing overrides on methods likepathToNodethat are derived fromid-> Madeidan abstract getter.Nodes with special hierarchy like
parentAutogroupNode, should only be concerned about theirnode.directVisibleChildren, and how it changes when it's expanded state is toggled. Prevents having to override more generic methods likenode.visibleChildrenthat return all embedded visible children under a node -> Renamednode.directChildrentonode.directVisibleChildrenand it respects the expanded state. Removed node.visibleChildren overrides. Much cleaner implementation.Removed redundant
node.matchByPathoverrides. Nodes won't match anyways, unless the correct path prefix is passed tobaseNode.matchByPath. The prefix is determined bynode.paththat uses the node's specific type and id.Cleaned up typescript errors from tests, since we no longer need to pass tree static methods down to
transactionNodes.