Repository navigation
feat: browser event summary in row side panel - #3442
SpencerTorres wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 7dad83d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const eventNameExpr = isLogSource(source) | ||
| ? source.eventNameExpression?.trim() | ||
| : undefined; | ||
| const eventNameColumn = |
There was a problem hiding this comment.
🟠 major — A bare event-name column that is ALIAS or MATERIALIZED never reaches the row, so detection quietly falls back to attribute keys
ClickHouse leaves ALIAS and MATERIALIZED columns out of SELECT * unless asterisk_include_alias_columns/asterisk_include_materialized_columns is set. That setting is only added when the user turns on the off-by-default option (useSelectAllColumnsQuerySettings in hooks/useMaterializedAliasColumnsOption.ts). So a source with eventNameExpression: 'EventName' on a table where EventName is e.g. ALIAS LogAttributes['event.name'] gets row.EventName === undefined. Every row then goes through the attribute fallback: browser.resource_timing/web_vital rows show 'No body for this event.' and navigations missing the fallback keys keep the HTTP Request section. Fix: decide between reading and selecting from the table's column metadata (getMetadata().getColumns, select it under the alias only when the column exists, whatever its kind). Or, at minimum, select the bare column under __hdx_event_name when metadata shows it as an ALIAS/MATERIALIZED column. Add a test for the case.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| attributes={highlightedAttributeValues} | ||
| mainContent={mainContent} | ||
| mainContentHeader={mainContentColumn} | ||
| hideEmptyBody={ |
There was a problem hiding this comment.
🔵 minor — The browser-row check for hideEmptyBody is written twice, in DBRowSidePanel and in RowOverviewPanel
DBRowSidePanelInner builds isLogSource(source) && isBrowserEventRow({eventName: normalizedRow[EVENT_NAME], attributes: normalizedRow[EVENT_ATTRIBUTES]}) inline. RowOverviewPanel (DBRowOverviewPanel.tsx:158-172) separately builds the same {eventName, attributes} row behind the same isLogSource gate. The side panel renders the header itself and passes hideHeader to the overview, so the two copies feed different headers and can drift apart. Fix: move it into one helper in utils/browserEvents.ts, e.g. isBrowserEventRowForSource(source, row) that reads __hdx_event_name/__hdx_event_attributes, and call it from both places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
 [Medium impact] The change affects browser log summaries across the app's row side-panel routes. The new changes look safe, but the previously reported fixture-typing requirement must still be met before merging.
 |
| // `any`, not `as`, keeps this partial fixture off the lint warning budget. | ||
| const asSource = (value: any): TSource => value; |
There was a problem hiding this comment.
The new asSource helper accepts any and returns TSource, so incomplete or wrongly shaped fixtures pass without a type check. This violates the repository guide's directive to avoid any and use proper typing. Give the fixtures an explicit checked type before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Deep Review✅ No critical issues found. No P0/P1: rendering is React-escaped (no XSS), non-string bodies are coerced before use, 🟡 P2 — recommended
🔵 P3 nitpicks (6)
Reviewers (9): correctness, adversarial, kieran-typescript, testing, maintainability, project-standards, previous-comments, agent-native, learnings-researcher. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
ef2d460 to
4bb67a0
Compare
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
| const eventNameExpr = isLogSource(source) | ||
| ? source.eventNameExpression?.trim() | ||
| : undefined; | ||
| const eventNameColumn = |
There was a problem hiding this comment.
🔵 minor — Bare event name column is lost when * or the Known Columns List leaves it out
SELECT * skips MATERIALIZED and ALIAS columns unless the viewer option is on (useSelectAllColumnsQuerySettings in hooks/useMaterializedAliasColumnsOption.ts), and a Known Columns List may simply not include EventName. In either case row[eventNameColumn] is undefined, so web-vital or resource-timing rows (which have no fallback keys) still show "No body for this event.". To fix it, check that the column exists using the table metadata and select it under __hdx_event_name when it does, rather than reading it off *. If you keep the current approach, add a test for an expression that is set but missing from knownColumns.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ['Message', body ? firstLine(body) : undefined], | ||
| ['Page URL', read(attributes, PAGE_URL_KEY)], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🔵 minor — Navigation URL row skips the browser.document.url.full fallback the event label uses
getNavigationLabel in utils/browserEvents.ts reads ['url.full', 'browser.document.url.full'], but this row only reads url.full. On a navigation row that carries only the document URL, the list shows "Navigated /checkout" while the panel shows no URL. If the flags are also missing, RequestDetailsTable returns null and the section is an empty "Browser event" accordion, and HTTP Request is hidden too. Use the same firstSetString(attributes, ['url.full', PAGE_URL_KEY]) lookup, and don't render the accordion item when it has no rows.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| attributes={highlightedAttributeValues} | ||
| mainContent={mainContent} | ||
| mainContentHeader={mainContentColumn} | ||
| hideEmptyBody={ |
There was a problem hiding this comment.
🔵 minor — Two copies of the empty-body decision, one per header caller
DBRowSidePanel and RowOverviewPanel each rebuild isLogSource(source) && isBrowserEventRow({eventName, attributes}) from the same row aliases before rendering DBRowSidePanelHeader. The two copies must be kept in sync by hand, and a third header caller would miss the rule. Move the check into one shared helper that takes (source, row) (or into the header, which already gets rowData), so every caller uses the same rule.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
E2E Test Results✅ All tests passed • 461 passed • 1 skipped • 1837s
Tests ran across 4 shards in parallel. |
4bb67a0 to
dc37768
Compare
| const eventNameExpr = isLogSource(source) | ||
| ? source.eventNameExpression?.trim() | ||
| : undefined; | ||
| const eventNameColumn = |
There was a problem hiding this comment.
🔵 minor — A bare event-name column that is ALIAS or MATERIALIZED never reaches the row
SELECT * leaves out ALIAS and MATERIALIZED columns unless the opt-in asterisk_include_* settings are on (hooks/useMaterializedAliasColumnsOption.ts). A Known Columns List that doesn't name the column also drops it. In those cases row[eventNameColumn] is undefined, so browser.resource_timing and browser.web_vital rows lose detection and show "No body for this event." again. Fix: when the source's columns are known to contain the identifier (via Metadata), select it under __hdx_event_name. At minimum, use the row value only when it is present and fall back to selecting it otherwise.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ['Message', body ? firstLine(body) : undefined], | ||
| ['Page URL', read(attributes, PAGE_URL_KEY)], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🔵 minor — Navigation URL row ignores the browser.document.url.full fallback the event label uses
getNavigationLabel in utils/browserEvents.ts reads ['url.full', 'browser.document.url.full']. This subpanel reads only url.full, so when url.full is missing the event list shows "Navigated " but the Browser event section shows no URL. Use the same key list here, e.g. firstSetString(attributes, ['url.full', PAGE_URL_KEY]), ideally through one shared constant.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| attributes={highlightedAttributeValues} | ||
| mainContent={mainContent} | ||
| mainContentHeader={mainContentColumn} | ||
| hideEmptyBody={ |
There was a problem hiding this comment.
🔵 minor — The browser-row predicate is built in two places that must stay in sync
isLogSource(source) && isBrowserEventRow({ eventName, attributes }) is written inline here and again in the useMemo in DBRowOverviewPanel.tsx:158. Each copy picks the event-name key its own way: one uses ROW_DATA_ALIASES.EVENT_NAME, the other the literal __hdx_event_name. Move both into one helper such as isBrowserEventLogRow(source, row) in utils/browserEvents.ts and call it from both components.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| )} | ||
|
|
||
| {requestPanelTitle != null && ( | ||
| {browserEventKind != null && ( |
There was a problem hiding this comment.
🔵 minor — The 'Browser event' section can open with nothing in it
The section shows whenever browserEventKind != null, but RequestDetailsTable (RequestDetailsTable.tsx:8) returns null when every row value is unset. A log source with an Event Name Expression but no eventAttributesExpression hits this: a browser.user_action.click row gets an expanded, empty 'Browser event' section. Build the rows in RowOverviewPanel, or have BrowserEventSubpanel report whether it has content, and render the Accordion.Item only when at least one row is set.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| attributes={highlightedAttributeValues} | ||
| mainContent={mainContent} | ||
| mainContentHeader={mainContentColumn} | ||
| hideEmptyBody={ |
There was a problem hiding this comment.
🔵 minor — The 'is this a browser log row' check is written out in two places
DBRowSidePanel repeats isLogSource(source) && isBrowserEventRow({eventName, attributes}) from RowOverviewPanel (DBRowOverviewPanel.tsx:158-172) to decide hideEmptyBody for the same header. The two copies can drift, for example if one gains a source-kind check and the other does not. Add one helper to utils/browserEvents.ts, such as isBrowserEventLogRow(source, row), and call it from both places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const eventNameExpr = isLogSource(source) | ||
| ? source.eventNameExpression?.trim() | ||
| : undefined; | ||
| const eventNameColumn = |
There was a problem hiding this comment.
🔵 minor — A bare EventName that is an ALIAS or MATERIALIZED column is never read
A bare identifier is only read from the SELECT * result. ClickHouse leaves ALIAS and MATERIALIZED columns out of * unless the asterisk_include_* settings are on, and useSelectAllColumnsQuerySettings (hooks/useMaterializedAliasColumnsOption.ts:46) only turns them on behind a user toggle. So an EventName ... ALIAS LogAttributes['event.name'] column gives an undefined event name. Detection then falls back to attribute keys, and browser.resource_timing and browser.web_vital rows show 'No body for this event.' again. Either read the column from the row only when data.meta lists it and select it under the alias otherwise, or note the limitation in the comment.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
552191c to
0b6835a
Compare
0b6835a to
053e6a7
Compare
053e6a7 to
302bb85
Compare
302bb85 to
4f33eb2
Compare
| const eventNameExpr = isLogSource(source) | ||
| ? source.eventNameExpression?.trim() | ||
| : undefined; | ||
| const eventNameColumn = |
There was a problem hiding this comment.
🔵 minor — A bare event-name column that is MATERIALIZED or ALIAS is never read
SELECT * leaves out MATERIALIZED/ALIAS columns unless the opt-in setting in hooks/useMaterializedAliasColumnsOption.ts is on. So with EventName ... MATERIALIZED LogAttributes['event.name'], row[eventNameColumn] is undefined and every non-click/nav/console browser.* row (web vitals, resource timing) loses its event name, so hideEmptyBody stays off for it. Fix: only take the bare-column shortcut when the column is known to come back from the row query (check the column kind via metadata, or check knownColumns). Otherwise select it under __hdx_event_name. Add a test for the case where the row has no such key but the column exists.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| attributes={highlightedAttributeValues} | ||
| mainContent={mainContent} | ||
| mainContentHeader={mainContentColumn} | ||
| hideEmptyBody={ |
There was a problem hiding this comment.
🔵 minor — Browser-row detection with the log-source gate is duplicated in two components
isLogSource(source) && isBrowserEventRow({eventName: row.__hdx_event_name, attributes: row.__hdx_event_attributes}) is now built separately here and in DBRowOverviewPanel.tsx:159-173. Both feed the same DBRowSidePanelHeader, which already receives rowData. Move this into one place so the two call sites can't drift. Either add a helper in utils/browserEvents.ts that takes (source, normalizedRow), or have the header work it out from rowData plus a source/isLogSource prop.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
9a382e6 to
7dad83d
Compare
karl-power
left a comment
There was a problem hiding this comment.
Just one comment here but otherwise looks good
Part of HDX-5736
Summary
Adds a browser event panel to the logs panel view. Newer OTel SDKs send these as logs, so this is a QoL change that adds clarity for logs that typically have no
BodytextChanges
BrowserEventSubpanel.tsx,DBRowOverviewPanel.tsx): "Browser event", open by default, after Exception.data-otel-*attribute, page URL.utils/browserEvents.ts):browser.user_action.click,browser.navigation,browser.console.browser.tag_name/browser.css_selector,browser.navigation.*,browser.console.method.DBRowSidePanelHeader.tsx,DBRowSidePanel.tsx): a newhideEmptyBodyprop hides "No body for this event." onbrowser.*log rows. A non-empty Body still shows. Backend logs andexceptionrows with no Body keep the message.DBRowDataPanel.tsx): no new query.EventNameis read from the row thatSELECT *or the Known Columns List already returns, not selected. Older OTel log tables have noEventNamecolumn, and selecting it would fail the whole lookup.__hdx_event_name.Screenshots
Browser event section in log panel:
