Skip to content
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

fix(trace): dont allow expanding and collapsing root node #79390

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 74 additions & 1 deletion static/app/views/performance/newTraceDetails/trace.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ async function keyboardNavigationTestSetup() {
makeTransaction({
span_id: i + '',
event_id: i + '',
transaction: 'transaction-name' + i,
transaction: 'transaction-name-' + i,
'transaction.op': 'transaction-op-' + i,
project_slug: 'project_slug',
})
Expand Down Expand Up @@ -275,6 +275,56 @@ async function pageloadTestSetup() {
return {...value, virtualizedContainer, virtualizedScrollContainer};
}

async function nestedTransactionsTestSetup() {
const transactions: TraceFullDetailed[] = [];

let txn = makeTransaction({
span_id: '0',
event_id: '0',
transaction: 'transaction-name-0',
'transaction.op': 'transaction-op-0',
project_slug: 'project_slug',
});

transactions.push(txn);

for (let i = 0; i < 100; i++) {
const next = makeTransaction({
span_id: i + '',
event_id: i + '',
transaction: 'transaction-name-' + i,
'transaction.op': 'transaction-op-' + i,
project_slug: 'project_slug',
});

txn.children.push(next);
txn = next;
transactions.push(next);

mockTransactionDetailsResponse(i.toString());
}

mockTraceResponse({
body: {
transactions: transactions,
orphan_errors: [],
},
});
mockTraceMetaResponse();
mockTraceRootFacets();
mockTraceRootEvent('0');
mockTraceEventDetails();
mockMetricsResponse();

const value = render(<TraceView />, {router});
const virtualizedContainer = getVirtualizedContainer();
const virtualizedScrollContainer = getVirtualizedScrollContainer();

// Awaits for the placeholder rendering rows to be removed
expect((await screen.findAllByText(/transaction-op-/i)).length).toBeGreaterThan(0);
return {...value, virtualizedContainer, virtualizedScrollContainer};
}

async function searchTestSetup() {
const transactions: TraceFullDetailed[] = [];
for (let i = 0; i < 11; i++) {
Expand Down Expand Up @@ -698,6 +748,7 @@ describe('trace view', () => {
await userEvent.keyboard('{arrowright}');
expect(await screen.findByText('special-span')).toBeInTheDocument();
});

it('arrow left collapses row', async () => {
const {virtualizedContainer} = await keyboardNavigationTestSetup();
const rows = virtualizedContainer.querySelectorAll(VISIBLE_TRACE_ROW_SELECTOR);
Expand All @@ -722,6 +773,28 @@ describe('trace view', () => {
expect(screen.queryByText('special-span')).not.toBeInTheDocument();
});

it('arrow left does not collapse trace root row', async () => {
const {virtualizedContainer} = await keyboardNavigationTestSetup();
const rows = virtualizedContainer.querySelectorAll(VISIBLE_TRACE_ROW_SELECTOR);

await userEvent.click(rows[0]);
await waitFor(() => expect(rows[0]).toHaveFocus());

await userEvent.keyboard('{arrowleft}');
expect(await screen.findByText('transaction-name-1')).toBeInTheDocument();
});

it('arrow left on transaction row still renders transaction children', async () => {
const {virtualizedContainer} = await nestedTransactionsTestSetup();
const rows = virtualizedContainer.querySelectorAll(VISIBLE_TRACE_ROW_SELECTOR);

await userEvent.click(rows[1]);
await waitFor(() => expect(rows[1]).toHaveFocus());

await userEvent.keyboard('{arrowleft}');
expect(await screen.findByText('transaction-name-2')).toBeInTheDocument();
});

it('roving updates the element in the drawer', async () => {
const {virtualizedContainer} = await keyboardNavigationTestSetup();
const rows = virtualizedContainer.querySelectorAll(VISIBLE_TRACE_ROW_SELECTOR);
Expand Down
3 changes: 3 additions & 0 deletions static/app/views/performance/newTraceDetails/trace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ export function Trace({
action_source: 'keyboard',
});
}

if (event.key === 'ArrowLeft') {
if (node.zoomedIn) {
onNodeZoomIn(event, node, false);
Expand All @@ -270,6 +271,8 @@ export function Trace({
} else if (event.key === 'ArrowRight') {
if (node.canFetch) {
onNodeZoomIn(event, node, true);
} else {
onNodeExpand(event, node, true);
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ describe('TraceTree', () => {
});
expect(tree.build().serialize()).toMatchSnapshot();
});

it('if parent span does not exist in span tree, the transaction stays under its previous parent', () => {
const tree = TraceTree.FromTrace(
makeTrace({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,11 @@ export class TraceTree extends TraceTreeEventDispatcher {
}

expand(node: TraceTreeNode<TraceTree.NodeValue>, expanded: boolean): boolean {
// Trace root nodes are not expandable or collapsable
if (isTraceNode(node)) {
return false;
}

// Expanding is not allowed for zoomed in nodes
if (expanded === node.expanded || node.zoomedIn) {
return false;
Expand Down Expand Up @@ -1255,7 +1260,12 @@ export class TraceTree extends TraceTreeEventDispatcher {
if (!expanded) {
const index = this.list.indexOf(node);
this.list.splice(index + 1, TraceTree.VisibleChildren(node).length);

node.expanded = expanded;
// When transaction nodes are collapsed, they still render child transactions
if (isTransactionNode(node)) {
this.list.splice(index + 1, 0, ...TraceTree.VisibleChildren(node));
}
} else {
node.expanded = expanded;
// Flip expanded so that we can collect visible children
Expand All @@ -1275,6 +1285,10 @@ export class TraceTree extends TraceTreeEventDispatcher {
organization: Organization;
}
): Promise<Event | null> {
if (isTraceNode(node)) {
return Promise.resolve(null);
}

if (zoomedIn === node.zoomedIn || !node.canFetch) {
return Promise.resolve(null);
}
Expand Down
Loading