Conversation
There was a problem hiding this comment.
Pull request overview
Adds transaction grouping to the web events table, with collapsible transaction summary rows and per-transaction color cues to make related queries easier to scan.
Changes:
- Widen the filter input to accommodate longer expressions.
- Render a transaction “summary” row (expand/collapse) when no filter is active, and indent child events.
- Apply a 6-color palette per transaction (summary row uses the color across all cells; child rows color the op column).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| web/static/style.css | Adds styling for transaction summary/child rows and tx color palette; widens filter input. |
| web/static/app.js | Implements tx grouping/collapsing logic, tx summary rendering, and tx color assignment/reset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
web/static/app.js
Outdated
| } | ||
| } | ||
| } else if (txId && seenTx.has(txId)) { | ||
| // Already handled by summary — skip | ||
| } else { | ||
| rows.push({kind: 'event', eventIdx: i}); | ||
| } | ||
| } | ||
| return rows; |
There was a problem hiding this comment.
The txSummaryInfo function doesn't handle empty indices array. If row.eventIndices is empty, accessing indices[0] and indices[indices.length - 1] will return undefined, causing subsequent operations to fail. While this might not occur in normal operation, defensive programming suggests adding a guard check at the start of the function.
| seenTx.add(txId); | ||
| const entry = txIndex.get(txId); | ||
| const indices = entry ? entry.indices : [i]; |
There was a problem hiding this comment.
Events with a tx_id but without a corresponding Begin event in the array will be displayed as individual rows (line 285). However, if these events are part of a transaction that was started before monitoring began, they might appear out of context. Consider adding visual indication or handling for orphaned transaction events to make this behavior clearer to users.
| for (const row of displayRows) { | ||
| if (row.kind === 'tx') { | ||
| const info = txSummaryInfo(row.eventIndices); | ||
| const collapsed = collapsedTx.has(row.txId); | ||
| const chevron = collapsed ? '\u25b8' : '\u25be'; |
There was a problem hiding this comment.
The chevron is purely visual without semantic meaning for screen readers. Consider adding an aria-label to the tx-summary row or wrapping the chevron in a button element with proper ARIA attributes to indicate its expandable/collapsible state for accessibility.
| const txId = ev.tx_id; | ||
| if (!txId) continue; | ||
| let entry = txIndex.get(txId); | ||
| if (!entry) { | ||
| entry = { indices: [] }; | ||
| txIndex.set(txId, entry); | ||
| } | ||
| entry.indices.push(i); |
There was a problem hiding this comment.
The logic assumes that transactions will always have a Begin event with op === 'Begin', and groups events starting from that Begin. However, if events are received out of order (e.g., Begin arrives after some other transaction events), or if Begin was missed, the transaction events might not be properly grouped. Consider whether this is the intended behavior or if there should be handling for out-of-order events.
| .sql-num { color: #b5cea8; } | ||
| .sql-param { color: #9cdcfe; } | ||
|
|
||
| /* Transaction grouping */ |
There was a problem hiding this comment.
Line 198 sets a default color for all tx-summary cells, but lines 211-216 override this with specific colors per data-tx-color attribute. The default color on line 198 will only apply when data-tx-color is not set (which should not happen based on the JS code). Consider removing the redundant color property from line 198 or clarifying its purpose with a comment if it's intentional as a fallback.
| /* Transaction grouping */ | |
| /* Transaction grouping */ | |
| /* Default/fallback color for tx-summary rows when data-tx-color is not set */ |
1 participant
No description provided.