Replays Self-Serve Bulk Delete System - #6
Conversation
This validates both the [Working Draft](https://www.w3.org/TR/reporting-1/#concept-reports) and the [Editor's Draft](https://w3c.github.io/reporting/#concept-reports) formats. Fixes [ID-730 - Accept current and upcoming data model](https://linear.app/getsentry/issue/ID-730/accept-current-and-upcoming-data-model).
…o 'low' (#93927)" This reverts commit 8d04522. Co-authored-by: roaga <47861399+roaga@users.noreply.github.com>
Missed in the initial commit, leading to some relevant logs being unannotated.
We have had a few tasks get killed at 10% rollout.
Also add a test, so that this doesn't happen again
Fixes DE-129 and DE-156 --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
These transitions should be matching
…` (#93946) Use `project_id` on the replay record instead of the URL (where it does not always exist). --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Also fixed `replay.view_html` -> `replay.view-html` --------- Co-authored-by: Michelle Zhang <56095982+michellewzhang@users.noreply.github.com>
…948) gets `npx @typescript/native-preview` passing again
This adds mode for all things tracing. This encompasses transactions/metrics/spans. Taken from https://github.com/getsentry/sentry/blob/feeaf393deeca8b97675bff23039c6320270aab5/src/sentry/runner/commands/devserver.py#L370
The conditions associated with a DCG can change over time, and it's good if we can be completely confident that they're consistent within a given task execution.
This is unused and most regex experiments have required broader changes to ensure that regexes are evaluated in a specific order (ex: traceparent). Removing this for now to simplify the code and very slightly improve runtime performance.
From some testing (on feedback lists of all different lengths), this prompt seems to work better. It doesn't write overly long sentences and also does a better job at "summarizing" versus just mentioning a few specific topics and leaving out others.
Just remove a couple custom Flex* classes in favor of the Flex primitive
This has been killed a few times. Refs SENTRY-42M7
…n table (#93892) <!-- Describe your PR here. --> [ticket](https://linear.app/getsentry/issue/ID-156/grouping-info-remove-type-field-from-ui) The Type field in the Grouping Info section of the issue details page was redundant. This removes the Type row from all variant types while keeping the underlying data structure intact. before  after 
### Changes Related to this PR: getsentry/sentry#93810. This is part 1 of the change, which is pulling out the new component and just adding it to the repo. Also includes some simplification of the logic in the base component. Part 2 will be replacing tables in widgets. ### Before/After There is no UI change as the table is not being used yet. There is a new story page for the component.
…93943) to prevent this issue from becoming too noisy, add a noise config
Unfortunately, 'event_data' went from being the variable for current event context to being the complete parsed data from Redis, and we continued logging it per group. That's more data than we should be logging even arguably once, let alone per group.
Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
Adds some simple analytics to our endpoint so we can begin building a dashboard in Amplitude.
Previously, explore supported multiple y axis per chart, so each visualize supported multiple y axis. That functionality has since been removed for simplicity so update the types here to match. Keep in mind that saved queries still store them as an array so when serializing/deserializing, we still need to treat it as an array.
We'll need the `useGetTraceItemAttributeKeys` hook in other places so refactoring it so that it can exported.
|
📉 Risk score updated: 93 → 91 (CRITICAL) Review above has been updated with the latest assessment. |
![re-entry-local[bot]](https://avatars.githubusercontent.com/in/3584620?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component is added, but the pre-fetched test index shows no existing tests for this file; add React tests to verify feature-flag gating, pending placeholder rendering, and error/too-few-feedback returning null.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
New useFeedbackSummary hook is added with API query enabling and tooFewFeedbacks derivation; the pre-fetched test index shows no existing tests for this file—add hook tests to cover enabled/disabled states and the numFeedbacksUsed===0 && !success condition.
| @@ -1,7 +1,7 @@ | |||
| import functools | |||
There was a problem hiding this comment.
New error-context fetching and response processing logic is added to the endpoint, but the pre-fetched test index shows no existing tests for this file; add regression tests for the new enable_error_context=false behavior and for malformed/empty Snuba responses.
| @@ -0,0 +1,104 @@ | |||
| import type {Theme} from '@emotion/react'; | |||
There was a problem hiding this comment.
New default head/body cell renderer logic is added (including tooltip fallback and fieldRenderer fallback); the pre-fetched test index shows no existing tests for this file—add unit tests for both head and body fallbacks, including the !tableData?.meta path.
|
➡️ Reassessed — no score change (91) (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -76,8 +75,10 @@ export class Visualize { | |||
| return json; | |||
There was a problem hiding this comment.
🚨 CRITICAL
Type/shape mismatch bug: Visualize.fromJSON now returns Visualize[] (array) but the method signature still expects a single Visualize in the rest of the codebase, which can cause runtime logic errors when callers treat the return as an object. Fix by updating the return type and all call sites to handle an array, or revert to returning a single Visualize and adjust serialization consistently.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added but the existing test coverage for this endpoint is not present in the Test Impact Index, so the new validation branches (e.g., timestamp vs age rules) may be under-tested. Add regression tests for invalid combinations like age present with timestamp, timestamp missing when required, and invalid type values to ensure 422 details match expectations.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
A new hook was added to fetch and interpret feedback summary API data, but the Test Impact Index shows no mapped tests, so edge cases like numFeedbacksUsed===0 && success===true/false and query enabling conditions may be unverified. Add hook tests that mock the API response for pending/error/success and assert tooFewFeedbacks and enabled behavior.
| @@ -0,0 +1,104 @@ | |||
| import type {Theme} from '@emotion/react'; | |||
There was a problem hiding this comment.
New default table cell renderer utilities were added (head/body fallback rendering, tooltip, and fieldRenderer usage) but there are no mapped tests in the Test Impact Index, so fallback behavior and alignment/units handling may be under-tested. Add unit tests for renderDefaultHeadCell tooltip rendering and renderDefaultBodyCell fallback paths when tableData.meta is missing vs present.
| @@ -139,6 +139,10 @@ class PullRequestFile: | |||
| patch: str | |||
There was a problem hiding this comment.
New PR comment template truncation and environment-info formatting were added, but there is no explicit regression test coverage evidence for the new helper methods in the Test Impact Index. Add unit tests for _truncate_title boundary lengths and for get_environment_info returning empty string when environment is missing/invalid, plus an end-to-end test asserting the new comment format includes in �0env�.
|
📈 Risk score updated: 91 → 93 (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -32,7 +32,7 @@ export function isGroupBy(value: any): value is GroupBy { | |||
| } | |||
There was a problem hiding this comment.
🚨 CRITICAL
Type/shape mismatch bug: isVisualize now checks for yAxis being a string, but other code paths in this diff still reference yAxes (e.g., updateLocationWithAggregateFields uses visualize.yAxes and isBaseVisualize still checks Array.isArray(value.yAxes)). This can cause incorrect type narrowing and wrong serialization of aggregate fields. Fix by updating all remaining yAxes usages to the new single yAxis shape (or adjusting the type guards consistently).
| @@ -37,36 +37,35 @@ export interface BaseVisualize { | |||
| export class Visualize { | |||
There was a problem hiding this comment.
🚨 CRITICAL
Possible: breaking TypeScript contract change — Visualize changed from yAxes: readonly string[] to yAxis: string, and fromJSON now returns Visualize[] (see visualizes.tsx:37 and fromJSON change). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all call sites were updated to the new constructor/fields and that any persisted JSON consumers handle the new fromJSON return type.
| @@ -80,7 +80,7 @@ export function getAggregateFieldsFromLocation( | |||
| } else if (isBaseVisualize(groupByOrBaseVisualize)) { | |||
There was a problem hiding this comment.
Logic bug / wrong constructor usage: updateLocationWithAggregateFields now uses flatMap and Visualize.fromJSON(aggregateField) but Visualize.fromJSON was changed to return Visualize[] (array). The surrounding code expects a list of JSON strings for location.query.aggregateField; if any branch returns the wrong nesting level, the query param format will be incorrect. Fix by ensuring each branch returns a consistent string[] and removing any extra array wrapping introduced by the refactor.
| @@ -20,15 +22,9 @@ interface TraceItemAttributeValue { | |||
| value: string; | |||
There was a problem hiding this comment.
Possible: hook contract change — useTraceItemAttributeValues was renamed to useGetTraceItemAttributeValues and its props changed (notably attributeKey removed; see useGetTraceItemAttributeValues.tsx:22). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all imports were updated and that any external consumers aren’t still using the old hook/props.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added but the existing test coverage for this endpoint is not present in the Test Impact Index (E1 shows no tests mapped). Add regression tests for invalid payload combinations (e.g., age present with timestamp, missing required fields, and attempts < 1) to ensure the new DRF serializer behavior returns 422 with correct details.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component was added and wired into the feedback list page, but there are no tests mapped for this file in the Test Impact Index (E4). Add component tests to cover feature-flag off, pending state (placeholder), error state (null), and success rendering of the summary text.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 93/100 · CRITICAL
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Implement a self-serve bulk delete system for replays and extend related replay summarization/error-context and browser reporting ingestion, with accompanying UI changes.
Summary
Behaviorally, this PR adds/changes backend flows for replay breadcrumb summarization (including optional error-context enrichment) and browser report ingestion (switching to a DRF serializer with structured 422s), plus new frontend UI for a gated “Feedback Summary”. The highest risks are (1) correctness/contract breakage from TypeScript shape changes around visualize JSON and (2) runtime crashes or silent misbehavior from missing null-guards in the new UI feature gating. Separately, the replay breadcrumb endpoint now performs additional data fetches without visible tracing/metrics, which can make performance regressions hard to diagnose. Before merging, verify the end-to-end request/response contracts for the browser reporting collector and replay breadcrumbs (both feature-flag branches), and confirm all visualize JSON parsing call sites were updated to the new yAxis/fromJSON return types.
🎯 Review Focus
The frontend visualize JSON contract changes (yAxis vs yAxes and fromJSON return type) and the new feedback UI feature gating—verify all call sites are updated and that organization.features is safely handled to prevent runtime crashes.
✅ Action Checklist
- SUGGESTION — src/sentry/issues/endpoints/browser_reporting_collector.py:L18 WARNING: Add explicit regression tests for the new DRF serializer validation behavior and response codes (422) for invalid payload combinations (e.g.,
agepresent withtimestamp, missing required fields, andattempts < 1) and assert the exact error detail structure; this endpoint’s validation semantics changed and the current test impact mapping indicates missing coverage. - SUGGESTION — src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L1 WARNING: Add tracing spans + counters around the new Snuba error-id query and nodestore error-context enrichment (and include gating behavior for
enable_error_context=false), so performance regressions and partial failures are observable; right now the added sub-fetches are not instrumented in the diff. - SUGGESTION — static/app/views/explore/contexts/pageParamsContext/aggregateFields.tsx:L80 WARNING: Ensure
updateLocationWithAggregateFieldsreturns a consistentstring[]forlocation.query.aggregateField; ifVisualize.fromJSONreturnsVisualize[], convert to the expected serialized string form in one place and remove any extra array wrapping introduced by the refactor.
Suggestions
- src/sentry/issues/endpoints/browser_reporting_collector.py:L18 WARNING: Add explicit regression tests for the new DRF serializer validation behavior and response codes (422) for invalid payload combinations (e.g.,
agepresent withtimestamp, missing required fields, andattempts < 1) and assert the exact error detail structure; this endpoint’s validation semantics changed and the current test impact mapping indicates missing coverage. - src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L1 WARNING: Add tracing spans + counters around the new Snuba error-id query and nodestore error-context enrichment (and include gating behavior for
enable_error_context=false), so performance regressions and partial failures are observable; right now the added sub-fetches are not instrumented in the diff. - static/app/views/explore/contexts/pageParamsContext/aggregateFields.tsx:L80 WARNING: Ensure
updateLocationWithAggregateFieldsreturns a consistentstring[]forlocation.query.aggregateField; ifVisualize.fromJSONreturnsVisualize[], convert to the expected serialized string form in one place and remove any extra array wrapping introduced by the refactor.
📝 This review includes 17 inline comments (2 critical, 10 warnings, 5 notes)
ℹ️ 11 additional lower-priority findings were hidden to keep this review focused. Adjust your team's review cap if you want them all.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -32,7 +32,7 @@ export function isGroupBy(value: any): value is GroupBy { | |||
| } | |||
There was a problem hiding this comment.
🚨 CRITICAL
Type/shape mismatch bug: isVisualize now checks for yAxis being a string, but other code paths in this diff still reference yAxes (e.g., updateLocationWithAggregateFields uses visualize.yAxes and isBaseVisualize still checks Array.isArray(value.yAxes)). This can cause incorrect type narrowing and wrong serialization of aggregate fields. Fix by updating all remaining yAxes usages to the new single yAxis shape (or adjusting the type guards consistently).
| @@ -37,36 +37,35 @@ export interface BaseVisualize { | |||
| export class Visualize { | |||
There was a problem hiding this comment.
🚨 CRITICAL
Possible: breaking TypeScript contract change — Visualize changed from yAxes: readonly string[] to yAxis: string, and fromJSON now returns Visualize[] (see visualizes.tsx:37 and fromJSON change). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all call sites were updated to the new constructor/fields and that any persisted JSON consumers handle the new fromJSON return type.
| @@ -80,7 +80,7 @@ export function getAggregateFieldsFromLocation( | |||
| } else if (isBaseVisualize(groupByOrBaseVisualize)) { | |||
There was a problem hiding this comment.
Logic bug / wrong constructor usage: updateLocationWithAggregateFields now uses flatMap and Visualize.fromJSON(aggregateField) but Visualize.fromJSON was changed to return Visualize[] (array). The surrounding code expects a list of JSON strings for location.query.aggregateField; if any branch returns the wrong nesting level, the query param format will be incorrect. Fix by ensuring each branch returns a consistent string[] and removing any extra array wrapping introduced by the refactor.
| @@ -20,15 +22,9 @@ interface TraceItemAttributeValue { | |||
| value: string; | |||
There was a problem hiding this comment.
Possible: hook contract change — useTraceItemAttributeValues was renamed to useGetTraceItemAttributeValues and its props changed (notably attributeKey removed; see useGetTraceItemAttributeValues.tsx:22). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all imports were updated and that any external consumers aren’t still using the old hook/props.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added but the existing test coverage for this endpoint is not present in the Test Impact Index (E1 shows no tests mapped). Add regression tests for invalid payload combinations (e.g., age present with timestamp, missing required fields, and attempts < 1) to ensure the new DRF serializer behavior returns 422 with correct details.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component was added and wired into the feedback list page, but there are no tests mapped for this file in the Test Impact Index (E4). Add component tests to cover feature-flag off, pending state (placeholder), error state (null), and success rendering of the summary text.
|
@CodingKylo I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 106 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx">
<violation number="1" location="static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx:31">
P2: Custom header cell overrides are gated by a truthy check, so valid falsy ReactNode values incorrectly trigger fallback rendering.</violation>
<violation number="2" location="static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx:70">
P2: Custom body cell overrides use a truthy check, causing falsy ReactNode results to be ignored and replaced by default rendering.</violation>
</file>
<file name="static/app/views/dashboards/widgetCard/chart.tsx">
<violation number="1" location="static/app/views/dashboards/widgetCard/chart.tsx:168">
P1: Feature-flagged table visualization drops all widget results by passing empty `tableData` and `columns`, so enabled orgs get blank tables.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Browser as Browser Client
participant BRAPI as Browser Reporting Collector Endpoint
participant SeerAPI as Seer API
participant NodeStore as NodeStore
participant Snuba as Snuba/ClickHouse
participant WorkflowEngine as Delayed Workflow Processor
participant AuthControl as Auth Control Tasks
participant WebhookCtrl as Webhook Delivery Tasks
participant GitHub as GitHub API
participant GitLab as GitLab API
participant Detectors as Detector API
Note over Browser,Detectors: Key Runtime Interactions
Browser->>BRAPI: POST /api/0/reporting-api-experiment (application/reports+json)
BRAPI->>BRAPI: Parse & validate with BrowserReportSerializer
alt Valid report
BRAPI->>BRAPI: Record metrics per report type
BRAPI-->>Browser: HTTP 200 OK
else Invalid report
BRAPI-->>Browser: HTTP 422 + validation errors
end
Note over Browser,Browser: Reporting API Spec: Working Draft uses "timestamp", Editor's Draft uses "age"
Note over WorkflowEngine: NEW: Slow condition group optimization
WorkflowEngine->>WorkflowEngine: get_slow_conditions_for_groups (moved outside loops)
WorkflowEngine->>WorkflowEngine: Check for DCGs without slow conditions
alt DCG has slow conditions
WorkflowEngine->>WorkflowEngine: Build condition query groups
WorkflowEngine->>Snuba: Execute condition queries
Snuba-->>WorkflowEngine: Query results
WorkflowEngine->>WorkflowEngine: Fire actions for matching groups
else No slow conditions
WorkflowEngine->>WorkflowEngine: Log no_slow_condition_groups
end
Note over SeerAPI: CHANGED: Replay breadcrumb analysis includes error context
Browser->>SeerAPI: POST /replay/analyze (logs + error messages)
SeerAPI-->>Browser: Analysis result
rect rgb(191, 223, 255)
Note over SeerAPI,NodeStore: CHANGED: Error event fetching flow
SeerAPI->>NodeStore: get_multi (fetch error event details)
NodeStore-->>SeerAPI: Event data (title, message, timestamp)
SeerAPI->>SeerAPI: Merge errors with breadcrumbs in chronological order
end
Note over GitHub,GitLab: CHANGED: PR comment format (bullet list, environment info, truncated titles)
Detectors->>Detectors: NEW: Owner field validation & assignment
alt Owner is user
Detectors->>Detectors: Set owner_user_id, clear owner_team_id
else Owner is team
Detectors->>Detectors: Set owner_team_id, clear owner_user_id
else Owner is None
Detectors->>Detectors: Clear both owner fields
end
Note over AuthControl: CHANGED: Added processing_deadline_duration=60s
Note over WebhookCtrl: CHANGED: Added deadline durations (30s, 120s, 300s)
Note over Snuba: CHANGED: equations parameter added to run_top_events_timeseries_query
Browser->>GitHub: POST comment with new bullet format + environment + truncated title
Browser->>GitLab: POST comment with new bullet format + environment + truncated title
Partial review: This PR has more than 50 files, so cubic reviewed the highest-priority files first. During the trial, paid plans get a higher file limit.
You can try an ultrareview to bypass the file limit, comment @cubic-dev-ai ultrareview. Learn more.
Fix all with cubic | Re-trigger cubic
| <TableWidgetVisualization | ||
| columns={[]} | ||
| tableData={{ | ||
| data: [], |
There was a problem hiding this comment.
P1: Feature-flagged table visualization drops all widget results by passing empty tableData and columns, so enabled orgs get blank tables.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At static/app/views/dashboards/widgetCard/chart.tsx, line 168:
<comment>Feature-flagged table visualization drops all widget results by passing empty `tableData` and `columns`, so enabled orgs get blank tables.</comment>
<file context>
@@ -162,23 +161,36 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
+ <TableWidgetVisualization
+ columns={[]}
+ tableData={{
+ data: [],
+ meta: {
+ fields: {},
</file context>
| rowIndex: number, | ||
| columnIndex: number | ||
| ): React.ReactNode { | ||
| const cell = renderTableBodyCell?.(column, dataRow, rowIndex, columnIndex); |
There was a problem hiding this comment.
P2: Custom body cell overrides use a truthy check, causing falsy ReactNode results to be ignored and replaced by default rendering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx, line 70:
<comment>Custom body cell overrides use a truthy check, causing falsy ReactNode results to be ignored and replaced by default rendering.</comment>
<file context>
@@ -0,0 +1,104 @@
+ rowIndex: number,
+ columnIndex: number
+ ): React.ReactNode {
+ const cell = renderTableBodyCell?.(column, dataRow, rowIndex, columnIndex);
+ if (cell) {
+ return cell;
</file context>
| column: TabularColumn<keyof TabularRow>, | ||
| _columnIndex: number | ||
| ): React.ReactNode { | ||
| const cell = renderTableHeadCell?.(column, _columnIndex); |
There was a problem hiding this comment.
P2: Custom header cell overrides are gated by a truthy check, so valid falsy ReactNode values incorrectly trigger fallback rendering.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx, line 31:
<comment>Custom header cell overrides are gated by a truthy check, so valid falsy ReactNode values incorrectly trigger fallback rendering.</comment>
<file context>
@@ -0,0 +1,104 @@
+ column: TabularColumn<keyof TabularRow>,
+ _columnIndex: number
+ ): React.ReactNode {
+ const cell = renderTableHeadCell?.(column, _columnIndex);
+ if (cell) {
+ return cell;
</file context>
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🚨 Risk Score: 94/100 · CRITICAL
| Dimension | Level |
|---|---|
| Likelihood | 🔴 Critical |
| Impact | 🔴 Critical |
| Detectability | 🟠 High |
Intent
Add self-serve replay bulk delete UI/flow while extending replay breadcrumb summarization with optional error-context enrichment and updating browser report ingestion/validation plus related frontend feedback summary components.
Summary
This PR introduces multiple cross-cutting behavior changes: it replaces the browser reporting collector’s dataclass parsing with a DRF serializer that returns structured 422 validation errors, and it extends the project replay breadcrumbs endpoint to optionally fetch and enrich error context via Snuba/nodestore. It also adds a new frontend “Feedback Summary” component/hook that calls a new API and gates rendering on an org feature flag. The highest risks to verify are (1) authorization/ownership invariants and contract changes in the new endpoints (especially 422 payload shape and breadcrumb response semantics), and (2) runtime correctness in the new React gating logic (feature flag access and null/undefined handling). Additionally, the diff shows removed/rewritten tests for browser reporting but no corresponding tests for the new replay breadcrumb error-context path—this is a likely source of silent regressions.
🎯 Review Focus
Verify endpoint contract + authorization invariants: (1) that owner assignment cannot be abused by unauthorized actors, and (2) that the new replay breadcrumbs and browser reporting collector response semantics (including 422 payload shape and optional error-context enrichment) are correct and covered by tests for empty/error/flag-off paths.
Key Findings
⚠️ [static/app/components/feedback/feedbackSummary.tsx:L18] WARNING: Feature-flag gating can crash iforganization.featuresis undefined—guard with optional chaining or default to an empty array before calling.includes, e.g.const features = organization.features ?? [];then usefeatures.includes(...).
✅ Action Checklist
- WARNING [static/app/components/feedback/feedbackSummary.tsx:L18] — Feature-flag gating can crash if
organization.featuresis undefined—guard with optional chaining or default to an empty array before calling.includes, e.g.const features = organization.features ?? [];then usefeatures.includes(...). - SUGGESTION — Add explicit authorization checks for owner assignment in the workflow engine path that consumes
BaseDetectorTypeValidatoroutput. Concretely: in the endpoint/service that appliesowner_user_id/owner_team_id, verifyrequest.user(or actor) has the required permission and that the target user/team is in the same organization before persisting. [src/sentry/workflow_engine/endpoints/validators/base/detector.py:L34] - SUGGESTION — For the replay breadcrumbs endpoint, add timing/throughput instrumentation and structured failure logging around the new Snuba/nodestore/error-context fetches (include
project_id,replay_id, andrequest.user.idor equivalent). Also add tests for the three scenarios:enable_error_context=false, Snuba returns no error_ids, and error-context fetch failure still yields a valid response. [src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L37] - SUGGESTION — Harden the React feature-flag gating to avoid runtime crashes: change
organization.features.includes(...)toBoolean((organization.features ?? []).includes('user-feedback-ai-summaries'))(or optional chaining). Add a unit test for the case wherefeaturesis missing. [static/app/components/feedback/feedbackSummary.tsx:L18] - SUGGESTION — Because the browser reporting collector now returns structured 422 errors via DRF serializer validation, add contract/regression tests that assert the exact error payload shape and details for both supported draft specs (and for invalid timestamp/age combinations). This is especially important since tests were restructured and the diff doesn’t show coverage for the new spec-compatibility rules. [src/sentry/issues/endpoints/browser_reporting_collector.py:L63]
📝 This review includes 10 inline comments (8 warnings, 2 notes)
ℹ️ Reviewed the highest-priority findings first — 6 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -32,7 +32,7 @@ export function isGroupBy(value: any): value is GroupBy { | |||
| } | |||
There was a problem hiding this comment.
🚨 CRITICAL
Type/shape mismatch bug: isVisualize now checks for yAxis being a string, but other code paths in this diff still reference yAxes (e.g., updateLocationWithAggregateFields uses visualize.yAxes and isBaseVisualize still checks Array.isArray(value.yAxes)). This can cause incorrect type narrowing and wrong serialization of aggregate fields. Fix by updating all remaining yAxes usages to the new single yAxis shape (or adjusting the type guards consistently).
| @@ -37,36 +37,35 @@ export interface BaseVisualize { | |||
| export class Visualize { | |||
There was a problem hiding this comment.
🚨 CRITICAL
Possible: breaking TypeScript contract change — Visualize changed from yAxes: readonly string[] to yAxis: string, and fromJSON now returns Visualize[] (see visualizes.tsx:37 and fromJSON change). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all call sites were updated to the new constructor/fields and that any persisted JSON consumers handle the new fromJSON return type.
| @@ -80,7 +80,7 @@ export function getAggregateFieldsFromLocation( | |||
| } else if (isBaseVisualize(groupByOrBaseVisualize)) { | |||
There was a problem hiding this comment.
Logic bug / wrong constructor usage: updateLocationWithAggregateFields now uses flatMap and Visualize.fromJSON(aggregateField) but Visualize.fromJSON was changed to return Visualize[] (array). The surrounding code expects a list of JSON strings for location.query.aggregateField; if any branch returns the wrong nesting level, the query param format will be incorrect. Fix by ensuring each branch returns a consistent string[] and removing any extra array wrapping introduced by the refactor.
| @@ -20,15 +22,9 @@ interface TraceItemAttributeValue { | |||
| value: string; | |||
There was a problem hiding this comment.
Possible: hook contract change — useTraceItemAttributeValues was renamed to useGetTraceItemAttributeValues and its props changed (notably attributeKey removed; see useGetTraceItemAttributeValues.tsx:22). Callers evidence is unavailable in the provided pre-fetch, so verify manually that all imports were updated and that any external consumers aren’t still using the old hook/props.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added but the existing test coverage for this endpoint is not present in the Test Impact Index (E1 shows no tests mapped). Add regression tests for invalid payload combinations (e.g., age present with timestamp, missing required fields, and attempts < 1) to ensure the new DRF serializer behavior returns 422 with correct details.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component was added and wired into the feedback list page, but there are no tests mapped for this file in the Test Impact Index (E4). Add component tests to cover feature-flag off, pending state (placeholder), error state (null), and success rendering of the summary text.
|
📈 Risk score updated: 93 → 94 (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report validation logic was added via a DRF serializer (including timestamp/age cross-field validation), but the pre-fetched test index shows no tests for this file. Add regression tests that cover invalid combinations (e.g., age present with timestamp, timestamp missing/negative, attempts < 1) and ensure the endpoint returns 422 with the expected error details.
| title: str | ||
| message: str | ||
| timestamp: float | ||
| category: str |
There was a problem hiding this comment.
The endpoint now fetches error IDs via Snuba and conditionally loads error event details, but the pre-fetched test index shows no tests for this file. Add tests for the new enable_error_context=false query param (ensuring no error events are fetched/returned) and for the Snuba/processing path when there are no error_ids.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
A new FeedbackSummary component was introduced and wired into the feedback list page, but the pre-fetched test index shows no tests for this component. Add unit tests for feature-flag gating, pending placeholder rendering, and error/too-few-feedback returning null.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
A new hook useFeedbackSummary was introduced to call the feedback-summary API and compute tooFewFeedbacks, but the pre-fetched test index shows no tests for this hook. Add hook tests that mock the API response shapes for success/error/pending and verify tooFewFeedbacks logic (e.g., numFeedbacksUsed===0 && !success).
|
➡️ Reassessed — no score change (94) (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -33,6 +34,7 @@ class BaseDetectorTypeValidator(CamelSnakeSerializer): | |||
| ) | |||
There was a problem hiding this comment.
Authorization gap / missing permission checks: the PR adds an owner field to BaseDetectorTypeValidator and updates Detector.owner_user_id / owner_team_id based on the provided actor identifier, but the diff does not show any authorization enforcement that the caller is allowed to set/change ownership. Mitigation: enforce role/permission checks in the endpoint/service layer (or in the validator using request context) to restrict who can assign owners and ensure the target actor belongs to the same organization.
| @@ -1,7 +1,7 @@ | |||
| import functools | |||
There was a problem hiding this comment.
The endpoint now performs additional network/data fetch steps (Snuba query + error context fetch) but the diff does not show any latency/throughput metrics or error-path logging around those calls. Add timing metrics for the overall request and for Snuba/error-context fetches, and log structured failures (including project_id, replay_id, and request.user.id) when Snuba/Seer/nodestore interactions fail.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added but the existing test coverage for this endpoint is not present in the Test Impact Index (and the diff only shows partial assertions). Add regression tests for the new spec-compatibility rules (timestamp vs age presence/absence) and for invalid payloads that should now return 422 with the expected details.
| title: str | ||
| message: str | ||
| timestamp: float | ||
| category: str |
There was a problem hiding this comment.
New error-context fetching logic was added (Snuba query + error event enrichment + enable/disable query param), but the Test Impact Index shows no tests for this endpoint. Add unit/integration tests for: (1) enable_error_context=false returns empty error events, (2) Snuba returns no error_ids, and (3) error fetching failure still returns a valid response.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🚨 Risk Score: 94/100 · CRITICAL
Intent
Implement a self-serve bulk delete system for replays, along with related replay breadcrumb/error-context enrichment and new browser reporting ingestion + UI feedback summary features.
Summary
This PR introduces multiple cross-cutting behavior changes: it switches browser report ingestion from a dataclass to a DRF serializer with new validation semantics and structured 422 responses, and it extends the project replay breadcrumbs endpoint to optionally fetch/enrich error context via Snuba/nodestore. It also adds new frontend UI/hooks for a gated “Feedback Summary” feature. The highest risks to verify are (1) contract/compatibility changes in request/response validation (browser reporting collector) and response composition (replays breadcrumbs), and (2) runtime correctness in the new React gating logic (feature flag checks) and the new endpoint’s error-context path. Additionally, the diff shows reduced/changed test coverage for the ingestion endpoint and no visible tests for the new replays error-context behavior—those gaps must be closed before merging.
🎯 Review Focus
Verify the new error-context enrichment path in project_replay_summarize_breadcrumbs.py is contract-stable and failure-tolerant (no crashes/partial responses) and that the browser reporting collector’s new DRF serializer validation/422 payload shape matches client expectations.
Key Findings
- 🚨 [static/app/components/feedback/feedbackSummary.tsx:L18] CRITICAL:
organization.features.includes(...)can throw iforganization.featuresis null/undefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;). ⚠️ [static/app/components/feedback/list/useFeedbackSummary.tsx:L33] WARNING:enabled: Boolean(normalizedDateRange) && organization.features.includes(...)can throw for the same null/undefinedorganization.featurescase and also couples query execution to a feature-flag check that may not be loaded yet; fix by usingconst features = organization.features ?? [];andenabled: Boolean(normalizedDateRange) && features.includes(...).
✅ Action Checklist
- CRITICAL [static/app/components/feedback/feedbackSummary.tsx:L18] —
organization.features.includes(...)can throw iforganization.featuresis null/undefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;). - WARNING [static/app/components/feedback/list/useFeedbackSummary.tsx:L33] —
enabled: Boolean(normalizedDateRange) && organization.features.includes(...)can throw for the same null/undefinedorganization.featurescase and also couples query execution to a feature-flag check that may not be loaded yet; fix by usingconst features = organization.features ?? [];andenabled: Boolean(normalizedDateRange) && features.includes(...). - SUGGESTION — Add regression tests for the browser reporting collector serializer covering the new working/editor draft combinations and mutual validation rules (age vs timestamp, attempts/timestamp min/max, invalid
type) and assert the exact 422 payload shape. File:src/sentry/issues/endpoints/browser_reporting_collector.py(serializer/validation section around L18-L120). - SUGGESTION — Add endpoint tests for
ProjectReplaySummarizeBreadcrumbsEndpointfor bothenable_error_context=falseand the error-fetching path (error_ids present, empty, and Snuba/nodestore failure). File:src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py(new error-context wiring around L37-L120). - SUGGESTION — Instrument the new external-call boundaries with spans/metrics so failures are observable (snuba query duration, nodestore lookup count, and whether error context was included). File:
src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py(around the newquery_replay_instance/process_raw_response/fetch_error_detailsflow). - SUGGESTION — Fix the React feature-flag gating to be null-safe and test it: add unit tests for
FeedbackSummaryanduseFeedbackSummarywhenorganization.featuresis undefined/null, and assert it rendersnullwithout crashing. Files:static/app/components/feedback/feedbackSummary.tsxandstatic/app/components/feedback/list/useFeedbackSummary.tsx. - SUGGESTION — If the browser reporting collector changed response semantics (e.g., from 200 with partial acceptance to 422 with structured errors), ensure any client(s) consuming it are updated or tolerant; add a contract test for the response payload schema. File:
src/sentry/issues/endpoints/browser_reporting_collector.py(Response/HTTP status handling around L18-L120).
📝 This review includes 8 inline comments (6 warnings, 2 notes)
ℹ️ Reviewed the highest-priority findings first — 4 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -1,7 +1,7 @@ | |||
| import functools | |||
There was a problem hiding this comment.
Test gap: the new error-context fetching path (Snuba query + process_raw_response + enable_error_context toggle) has no tests listed in the Test Impact Index. Add tests for (1) enable_error_context=false returning empty error events and (2) Snuba returning no rows / missing error_ids to ensure the endpoint still returns valid breadcrumbs.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
Test gap: the new FeedbackSummary component has no tests listed in the Test Impact Index, so its feature-flag gating, pending placeholder, and error/tooFewFeedbacks null behavior are under-tested. Add component tests to assert it renders null when feature disabled/tooFewFeedbacks/isError, shows Placeholder when pending, and renders summary text when successful.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
Test gap: the new useFeedbackSummary hook has no tests listed in the Test Impact Index, leaving query enabling and tooFewFeedbacks derivation unverified. Add hook tests that mock useApiQuery to cover pending, error, success with numFeedbacksUsed=0 & success=false (tooFewFeedbacks=true), and success with numFeedbacksUsed>0.
| @@ -0,0 +1,104 @@ | |||
| import type {Theme} from '@emotion/react'; | |||
There was a problem hiding this comment.
Test gap: defaultTableCellRenderers (head/body fallback rendering, tooltip, and fieldRenderer/unit wiring) has no tests listed in the Test Impact Index. Add unit tests for renderDefaultHeadCell fallback tooltip/align and renderDefaultBodyCell fallback using getFieldRenderer with meta.fields/meta.units.
|
➡️ Reassessed — no score change (94) (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added (working draft + editor draft fields, timestamp/age mutual validation), but the pre-fetched evidence shows no existing tests for this file. Add regression tests covering valid/invalid combinations for age vs timestamp, invalid type values, and min/max constraints for attempts/timestamp to ensure the new serializer behavior is enforced.
| @@ -25,6 +29,14 @@ | |||
| logger = logging.getLogger(__name__) | |||
There was a problem hiding this comment.
New error-context fetching/processing logic was added (Snuba query, process_raw_response, fetch_error_details gating via enable_error_context). Pre-fetched evidence indicates no existing tests for this endpoint file, so add unit/integration tests for both enable_error_context=false (no error events) and the error-fetching path (error_ids -> error events) including empty/error responses.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component was introduced, but pre-fetched evidence shows no tests for this file. Add component tests to verify feature-flag gating (user-feedback-ai-summaries), pending placeholder rendering, and error/too-few-feedbacks returning null.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
New useFeedbackSummary hook was introduced with query enabling logic and tooFewFeedbacks derivation from numFeedbacksUsed/success. Pre-fetched evidence shows no tests for this file; add hook tests that mock the API response shapes to cover pending, error, success with summary, and the tooFewFeedbacks condition.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🚨 Risk Score: 94/100 · CRITICAL
Intent
Add self-serve replay breadcrumb summarization with optional error-context enrichment and introduce a new UI feature for AI-generated feedback summaries.
Summary
The PR adds a new DRF serializer-based browser reporting collector validation path (including 422 structured errors) and extends the project replay summarize-breadcrumbs endpoint to optionally fetch and inject error-context details into the response. It also introduces new frontend React components/hooks for rendering a “Feedback Summary” based on a new API endpoint, gated by an org feature flag. The highest risks are (1) correctness/performance regressions from error-context enrichment that can fan out per error id without hard caps, and (2) client compatibility changes from altered validation semantics and response payloads. Verify that the replay endpoint has bounded work and adequate tests for both enable_error_context=false and error-fetch failure/empty cases, and that the browser reporting collector’s new serializer validation matches both W3C draft variants without breaking existing clients.
🎯 Review Focus
The replay breadcrumb endpoint’s optional error-context enrichment: verify it cannot cause unbounded work (fan-out per error id), that it behaves correctly when enable_error_context=false, and that error-fetch failures/empty results are handled deterministically with tests and instrumentation.
✅ Action Checklist
- SUGGESTION — Add a hard cap and batch fetch for error-context enrichment in
src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.pyaround the Snuba error_ids -> error event details flow (e.g.,error_ids = error_ids[:MAX]and fetch details with a singleWHERE id IN (...)query). Also add tests forenable_error_context=false(no Snuba/error fetch calls) and for empty/error-fetch failure paths. - SUGGESTION — In
src/sentry/issues/endpoints/browser_reporting_collector.py, add DRF serializer tests covering both W3C draft variants (timestamp vs age) and invalid combinations (both present / neither present). Assert the response status is 422 and that the error payload shape is stable for clients. - SUGGESTION — In
src/sentry/workflow_engine/processors/delayed_workflow.py, add metrics for delayed workflow processing: total processed, total failed, and processing latency (and ideally per-exception-type). Ensure the metrics are emitted in both success and exception paths, not only whenno_slow_condition_groupsoccurs.
📝 This review includes 9 inline comments (9 warnings)
ℹ️ Reviewed the highest-priority findings first — 5 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added (working draft + editor draft fields, timestamp/age mutual validation), but the pre-fetched evidence shows no existing tests for this file. Add regression tests covering valid/invalid combinations for age vs timestamp, invalid type values, and min/max constraints for attempts/timestamp to ensure the new serializer behavior is enforced.
| @@ -25,6 +29,14 @@ | |||
| logger = logging.getLogger(__name__) | |||
There was a problem hiding this comment.
New error-context fetching/processing logic was added (Snuba query, process_raw_response, fetch_error_details gating via enable_error_context). Pre-fetched evidence indicates no existing tests for this endpoint file, so add unit/integration tests for both enable_error_context=false (no error events) and the error-fetching path (error_ids -> error events) including empty/error responses.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
New FeedbackSummary component was introduced, but pre-fetched evidence shows no tests for this file. Add component tests to verify feature-flag gating (user-feedback-ai-summaries), pending placeholder rendering, and error/too-few-feedbacks returning null.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
New useFeedbackSummary hook was introduced with query enabling logic and tooFewFeedbacks derivation from numFeedbacksUsed/success. Pre-fetched evidence shows no tests for this file; add hook tests that mock the API response shapes to cover pending, error, success with summary, and the tooFewFeedbacks condition.
|
➡️ Reassessed — no score change (94) (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -650,6 +649,18 @@ def process_delayed_workflows( | |||
|
|
|||
| workflows_to_envs = fetch_workflows_envs(list(event_data.workflow_ids)) | |||
There was a problem hiding this comment.
process_delayed_workflows adds an info log for no_slow_condition_groups, but the diff does not add job-level success/failure metrics for the delayed workflow processing itself (e.g., how many workflows processed, how many failed, and processing latency). Add metrics around the overall processing loop and around any exception handlers so operators can alert on silent failures.
| @@ -52,17 +64,117 @@ def get(self, request: Request, project, replay_id: str) -> Response: | |||
| ): | |||
There was a problem hiding this comment.
Possible N+1 / unbounded fan-out on the request hot path: the endpoint now fetches error context by first querying error_ids via Snuba and then fetching error event details (looping over error_ids) unless enable_error_context=false. Latency will scale with the number of errors in the replay time window; fix by batching error event fetches (single query by ids) and/or enforcing a hard cap/pagination on error_ids processed per request.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added, but the pre-fetched evidence shows no existing tests for this endpoint file; add regression tests for both W3C draft variants (timestamp vs age) and invalid combinations (e.g., both present / neither present) to ensure the new validate_* methods behave correctly.
| @@ -25,6 +29,14 @@ | |||
| logger = logging.getLogger(__name__) | |||
There was a problem hiding this comment.
New error-context fetching and response processing logic was added (Snuba query + process_raw_response + optional enable_error_context flag), but pre-fetched evidence indicates no existing tests for this endpoint file; add unit/integration tests covering enable_error_context=false (no error fetch) and the error-fetch failure/empty error_ids path.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🚨 Risk Score: 94/100 · CRITICAL
Intent
Add self-serve bulk delete support for replays while enriching browser reporting validation and replay breadcrumb responses with optional error context, plus introduce a new UI component to display AI-generated feedback summaries.
Summary
This PR introduces a DRF serializer-based browser reporting collector with new cross-field validation and structured 422 responses, and it extends the project replay breadcrumbs endpoint to optionally fetch and inject error context derived from Snuba/error events. It also adds a new UI hook/component for “Feedback Summary” driven by a new API query. The highest risks are (1) behavioral contract changes in request validation/response payloads that can break existing clients and (2) correctness/performance risks in the replay breadcrumb endpoint due to additional fan-out queries and error-derived message injection. Please verify that the new validation rules and response shapes are backward-compatible (or versioned), and that the replay breadcrumb endpoint is bounded, tested, and safe when error context is enabled/disabled or when upstream data is missing/fails.
🎯 Review Focus
The replay breadcrumbs endpoint’s optional error-context enrichment: verify it is bounded (no unbounded fan-out), safe when upstream data is missing/fails, and that the response contract/payload shape is stable when enable_error_context is toggled.
✅ Action Checklist
- SUGGESTION — Add endpoint-level tests for
ProjectReplaySummarizeBreadcrumbsEndpointcoveringenable_error_context=false, Snuba returning noerror_ids, and error-event fetch failures/empty results. Concretely, create tests that assert the response contains breadcrumbs without any error-context fields when disabled, and that it still returns 200 (or the intended status) when upstream error detail fetch returns empty/raises. [src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L37] - SUGGESTION — Bound and instrument error-context enrichment: enforce a maximum number of error IDs processed (e.g.,
MAX_ERROR_CONTEXT_IDS) and add metrics/timing around the Snuba call and the error-context fetch path so you can detect latency spikes and failure rates. This should be implemented directly around the Snuba query and the subsequent error detail fetch. [src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L37] - SUGGESTION — For the browser reporting collector, add explicit tests asserting the serializer’s cross-field validation behavior and the exact 422 payload shape. Include cases:
agepresent withtimestamp,timestamppresent withage,attempts< 1, and invalidtypevalues; also verify behavior for both supported draft-spec modes. [src/sentry/issues/endpoints/browser_reporting_collector.py:L18] - SUGGESTION — If the replay breadcrumb endpoint injects error-derived strings into the response, ensure those strings are treated as plain text (not HTML) end-to-end and are not later rendered unsafely by the frontend. Add a test that includes an error message containing characters like
<script>and assert the frontend renders it as text (or the API escapes/normalizes it). [src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py:L64]
📝 This review includes 10 inline comments (9 warnings, 1 note)
ℹ️ Reviewed the highest-priority findings first — 6 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -650,6 +649,18 @@ def process_delayed_workflows( | |||
|
|
|||
| workflows_to_envs = fetch_workflows_envs(list(event_data.workflow_ids)) | |||
There was a problem hiding this comment.
process_delayed_workflows adds an info log for no_slow_condition_groups, but the diff does not add job-level success/failure metrics for the delayed workflow processing itself (e.g., how many workflows processed, how many failed, and processing latency). Add metrics around the overall processing loop and around any exception handlers so operators can alert on silent failures.
| @@ -52,17 +64,117 @@ def get(self, request: Request, project, replay_id: str) -> Response: | |||
| ): | |||
There was a problem hiding this comment.
Possible N+1 / unbounded fan-out on the request hot path: the endpoint now fetches error context by first querying error_ids via Snuba and then fetching error event details (looping over error_ids) unless enable_error_context=false. Latency will scale with the number of errors in the replay time window; fix by batching error event fetches (single query by ids) and/or enforcing a hard cap/pagination on error_ids processed per request.
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report serializer/validation logic was added, but the pre-fetched evidence shows no existing tests for this endpoint file; add regression tests for both W3C draft variants (timestamp vs age) and invalid combinations (e.g., both present / neither present) to ensure the new validate_* methods behave correctly.
| @@ -25,6 +29,14 @@ | |||
| logger = logging.getLogger(__name__) | |||
There was a problem hiding this comment.
New error-context fetching and response processing logic was added (Snuba query + process_raw_response + optional enable_error_context flag), but pre-fetched evidence indicates no existing tests for this endpoint file; add unit/integration tests covering enable_error_context=false (no error fetch) and the error-fetch failure/empty error_ids path.
|
➡️ Reassessed — no score change (94) (CRITICAL) Review above has been updated with the latest assessment. |
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
New browser report validation logic was added via a DRF serializer (including timestamp/age cross-field validation), but the pre-fetched test index shows no tests cover this file. Add regression tests for invalid combinations (e.g., age present with timestamp, timestamp present with age, attempts < 1, and invalid type values) to ensure the new serializer behavior is enforced.
| title: str | ||
| message: str | ||
| timestamp: float | ||
| category: str |
There was a problem hiding this comment.
The endpoint now fetches error IDs via Snuba and conditionally loads error event details (including a query param to disable error context), but the pre-fetched test index shows no tests cover this file. Add tests for: (1) enable_error_context=false returns breadcrumbs without error context, (2) Snuba returns no error_ids, and (3) error event fetch failure/empty results are handled gracefully.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
A new FeedbackSummary component and its feature-flag/pending/error rendering logic were added, but the pre-fetched test index shows no tests cover this file. Add component tests to verify it renders null when the feature flag is off or when tooFewFeedbacks/isError are true, shows a placeholder while pending, and renders the summary when successful.
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
A new hook useFeedbackSummary was added with query enabling logic and a tooFewFeedbacks computation based on numFeedbacksUsed and success, but the pre-fetched test index shows no tests cover this file. Add hook tests to cover enabled/disabled states, pending/error branches, and the tooFewFeedbacks truth table (including success=false with numFeedbacksUsed=0).
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 95/100 · CRITICAL
Intent
Implement a self-serve bulk delete system for replays and extend related replay breadcrumb/error-context and browser reporting ingestion/validation, with supporting frontend UI changes.
Summary
Behaviorally, this PR changes the replay breadcrumb summarization endpoint to optionally enrich responses with error context fetched from Snuba/nodestore, and it switches the browser reporting collector endpoint from a dataclass-based parser to DRF serializer-based validation with structured 422 responses. It also adds a new gated “Feedback Summary” frontend component/hook that calls a new API endpoint and conditionally renders based on org feature flags and query state. The highest risks to verify are (1) contract/compatibility and correctness of the new browser reporting validation/response semantics and (2) performance and failure-mode behavior of the new error-context enrichment path in the replays breadcrumbs endpoint (including whether it can fan out or degrade latency). Additionally, the frontend feature gating uses organization.features.includes(...) without guarding for features being undefined, which can cause runtime crashes.
🎯 Review Focus
Verify the new browser reporting collector validation contract (DRF serializer semantics + 422 payload shape) and ensure the frontend feature gating around organization.features is null-safe to prevent runtime crashes.
Key Findings
- 🚨 [static/app/components/feedback/feedbackSummary.tsx:L18] CRITICAL:
organization.features.includes(...)is called without guarding thatorganization.featuresis defined, so iffeaturesis null/undefined for any org payload the component will throw and break the page; fix by using optional chaining and a safe default (e.g.,const hasFeature = organization.features?.includes('user-feedback-ai-summaries') ?? false;). ⚠️ [static/app/components/feedback/list/useFeedbackSummary.tsx:L33] WARNING: TheuseApiQueryenabledcondition usesorganization.features.includes(...)without guardingorganization.features, which can crash during render/effect setup; fix by usingorganization.features?.includes(...) ?? false(and ideally centralize the feature check).⚠️ [src/sentry/issues/endpoints/browser_reporting_collector.py:L18] WARNING: The endpoint replaces the previous dataclass parsing with a DRFBrowserReportSerializerand changes imports/response handling to DRFResponse/status codes, but the diff excerpt doesn’t show explicit handling for unknown/invalid report types or cross-field validation behavior parity; fix by adding/ensuring tests that assert the exact 422 payload shape and that invalidtypevalues are rejected consistently with the old behavior (or document the breaking change).
✅ Action Checklist
- CRITICAL [static/app/components/feedback/feedbackSummary.tsx:L18] —
organization.features.includes(...)is called without guarding thatorganization.featuresis defined, so iffeaturesis null/undefined for any org payload the component will throw and break the page; fix by using optional chaining and a safe default (e.g.,const hasFeature = organization.features?.includes('user-feedback-ai-summaries') ?? false;). - WARNING [static/app/components/feedback/list/useFeedbackSummary.tsx:L33] — The
useApiQueryenabledcondition usesorganization.features.includes(...)without guardingorganization.features, which can crash during render/effect setup; fix by usingorganization.features?.includes(...) ?? false(and ideally centralize the feature check). - WARNING [src/sentry/issues/endpoints/browser_reporting_collector.py:L18] — The endpoint replaces the previous dataclass parsing with a DRF
BrowserReportSerializerand changes imports/response handling to DRFResponse/status codes, but the diff excerpt doesn’t show explicit handling for unknown/invalid report types or cross-field validation behavior parity; fix by adding/ensuring tests that assert the exact 422 payload shape and that invalidtypevalues are rejected consistently with the old behavior (or document the breaking change). - SUGGESTION — Add regression tests for the new DRF serializer validation branches in
src/sentry/issues/endpoints/browser_reporting_collector.py(e.g., bothageandtimestamppresent,attempts=0, unknowntype, and any mutual exclusivity rules). Ensure tests assert both HTTP status (422) and the response body structure (field names + error messages). - SUGGESTION — In
static/app/components/feedback/feedbackSummary.tsx:L18, change the feature gate to be null-safe:const hasAiSummaries = organization.features?.includes('user-feedback-ai-summaries') ?? false;and use!hasAiSummariesin the early return condition. - SUGGESTION — In
static/app/components/feedback/list/useFeedbackSummary.tsx:L33, make the queryenabledcondition null-safe and consistent with the component gate:enabled: Boolean(normalizedDateRange) && (organization.features?.includes('user-feedback-ai-summaries') ?? false). Consider extracting a sharedhasAiSummarieshelper to avoid divergence. - SUGGESTION — For
src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py, add explicit tests for both branches of the new error-context enrichment (when the query param disables fetching vs default behavior). Also add performance/error-path instrumentation around the Snuba/nodestore calls so failures don’t silently degrade latency or ordering.
📝 This review includes 11 inline comments (11 warnings)
ℹ️ Reviewed the highest-priority findings first — 7 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| @@ -17,30 +18,46 @@ | |||
|
|
|||
There was a problem hiding this comment.
Possible: BrowserReportSerializer introduces new validation logic (timestamp/age mutual exclusivity, attempts min_value, type choices) but the pre-fetched evidence shows no existing tests are mapped to this file. Add regression tests for invalid payloads covering the new validate_timestamp/validate_age branches (e.g., both age and timestamp present, age present with timestamp present, attempts=0, unknown type).
| @@ -52,17 +64,117 @@ def get(self, request: Request, project, replay_id: str) -> Response: | |||
| ): | |||
There was a problem hiding this comment.
Possible: The endpoint now fetches error_ids via Snuba and conditionally fetches error details based on enable_error_context, but the pre-fetched evidence shows no tests are mapped to this file. Add tests for both branches: enable_error_context=false returns breadcrumbs without error context, and the default path includes error events when Snuba returns error_ids.
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
Possible: New FeedbackSummary component renders different UI states (null when feature disabled/tooFewFeedbacks/isError, placeholder when pending, summary when loaded) but no tests are mapped to this file. Add unit tests for each state by mocking useFeedbackSummary/useOrganization and asserting rendered output (null vs Placeholder vs summary text).
|
|
||
| const {selection} = usePageFilters(); | ||
|
|
||
| const normalizedDateRange = normalizeDateTimeParams(selection.datetime); |
There was a problem hiding this comment.
Possible: New hook useFeedbackSummary computes tooFewFeedbacks and handles pending/error states, but no tests are mapped to this file. Add hook tests covering: enabled=false (query not fired), pending returns isPending=true, isError returns isError=true, and tooFewFeedbacks is true only when numFeedbacksUsed===0 && !success.
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
🚨 Risk Score: 95/100 · CRITICAL
Intent
Implement a self-serve replay bulk delete flow and extend related replay breadcrumb/error-context and browser reporting ingestion, alongside new gated UI for AI feedback summaries.
Summary
This change introduces backend behavior updates for replay breadcrumb summarization (optional error-context enrichment via Snuba/nodestore) and switches the browser reporting collector endpoint from a dataclass parser to a DRF serializer with structured 422 responses. It also adds a new frontend “Feedback Summary” component/hook gated by an org feature flag. The highest risks to verify are (1) correctness/contract stability of the new endpoint response shapes under edge cases (no error IDs, enable_error_context=false, and validation failures) and (2) runtime safety in the new React feature gating (feature flag checks and null/undefined handling). Additionally, the diff shows multiple TypeScript correctness warnings around falsy ReactNode handling and visualization contract changes that can cause subtle UI regressions.
🎯 Review Focus
Verify runtime safety and contract stability in the new/changed endpoints and UI gating: specifically organization.features.includes(...) in FeedbackSummary and the replay breadcrumbs/browser reporting collector response shapes under edge cases (no error IDs, enable_error_context=false, and DRF 422 validation failures).
Key Findings
- 🚨 [static/app/components/feedback/feedbackSummary.tsx:L14] CRITICAL:
organization.features.includes(...)can throw iforganization.featuresisundefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;).
✅ Action Checklist
- CRITICAL [static/app/components/feedback/feedbackSummary.tsx:L14] —
organization.features.includes(...)can throw iforganization.featuresisundefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;). - SUGGESTION — Add a regression test for
FeedbackSummarywhenorganization.featuresis missing/undefined to ensure it returns null instead of crashing:static/app/components/feedback/feedbackSummary.tsx(around theincludescheck). - SUGGESTION — For
defaultTableCellRenderers.tsx, add unit tests covering falsy-but-valid ReactNodes for both head and body renderers (0/false/''), and assert the custom renderer is used instead of the fallback:static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx. - SUGGESTION — For
browser_reporting_collector.py, add/extend tests to lock the request/response contract for the new DRF serializer path: verify 422 payload structure and cross-field validation (e.g.,ageandtimestampmutually exclusive;attempts < 1rejected) and ensure 200 responses still match the previous schema:src/sentry/issues/endpoints/browser_reporting_collector.py. - SUGGESTION — For
project_replay_summarize_breadcrumbs.py, add tests for response stability whenenable_error_context=falseand when Snuba returns noerror_ids(ensure the merged chronological log/error list and pagination metadata remain consistent):src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py.
📝 This review includes 17 inline comments (17 warnings)
ℹ️ Reviewed the highest-priority findings first — 13 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| ): React.ReactNode { | ||
| const cell = renderTableHeadCell?.(column, _columnIndex); | ||
| if (cell) { | ||
| return cell; |
There was a problem hiding this comment.
TS1 truthy check on a ReactNode-typed value: if (cell) { return cell; } will incorrectly treat valid falsy ReactNodes (e.g. 0, false, '') as “no cell” and fall back. Fix by checking cell !== undefined && cell !== null (or cell == null).
|
|
||
| def validate_timestamp(self, value: int) -> int: | ||
| """Validate that age is absent, but timestamp is present.""" | ||
| if self.initial_data.get("age"): |
There was a problem hiding this comment.
Wrong validation logic in validate_timestamp: it uses if self.initial_data.get("age"): which treats age=0 as absent and will raise/allow incorrectly depending on spec expectations. Fix by checking key presence explicitly (e.g. "age" in self.initial_data).
| ); | ||
| } | ||
|
|
||
| const SummaryContainer = styled('div')` |
There was a problem hiding this comment.
A new FeedbackSummary component and its feature-gated rendering logic were added, but the pre-fetched test index shows no existing tests mapped to this file. Add component tests to verify: returns null when feature flag is off/too few feedbacks/isError, shows placeholder while pending, and renders summary text when successful.
| @@ -0,0 +1,67 @@ | |||
| import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilters/parse'; | |||
There was a problem hiding this comment.
A new hook was added that drives API querying and computes tooFewFeedbacks from numFeedbacksUsed and success, but the pre-fetched test index shows no existing tests mapped to this file. Add hook tests to cover pending/error/success states and the tooFewFeedbacks boolean logic for both success=true and success=false cases.
| ): React.ReactNode { | ||
| const cell = renderTableHeadCell?.(column, _columnIndex); | ||
| if (cell) { | ||
| return cell; |
There was a problem hiding this comment.
Bug class: React rendering logic. renderDefaultHeadCell uses a truthy check (if (cell)) on renderTableHeadCell?.(...) which is typed as React.ReactNode | undefined; this will incorrectly treat valid falsy ReactNodes (e.g. 0, false, '') as “no custom cell” and fall back. Fix by using an explicit null/undefined check (cell !== undefined && cell !== null).
| if (cell) { | ||
| return cell; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug class: React rendering logic. renderDefaultBodyCell uses a truthy check (if (cell)) on renderTableBodyCell?.(...) which is typed as React.ReactNode | undefined; this can incorrectly fall back for falsy ReactNodes. Fix by using cell !== undefined && cell !== null before returning.
| @@ -32,7 +32,7 @@ export function isGroupBy(value: any): value is GroupBy { | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Bug class: Type/shape mismatch leading to wrong runtime behavior. isVisualize now checks for 'yAxis' in value and typeof value.yAxis === 'string', but other code paths in the diff still reference yAxes (e.g., getAggregateFieldsFromLocation iterates groupByOrBaseVisualize.yAxes in the truncated context). If any callers still pass the old {yAxes: [...]} shape, isVisualize will return false and those visualizations will be treated as non-visualize fields. Fix by ensuring all call sites are migrated to the new yAxis shape or by supporting both shapes during transition.
| @@ -52,17 +64,117 @@ def get(self, request: Request, project, replay_id: str) -> Response: | |||
| ): | |||
There was a problem hiding this comment.
Bug class: Possible None/empty response handling. The new code sets error_ids = response[0].get("error_ids", []) if response else []; if process_raw_response returns a non-empty list whose first element is None or not a dict, .get will raise. Fix by validating response[0] is a mapping (or using optional chaining / type guards) before calling .get.
| ): React.ReactNode { | ||
| const cell = renderTableHeadCell?.(column, _columnIndex); | ||
| if (cell) { | ||
| return cell; |
There was a problem hiding this comment.
TS2 rule: renderDefaultHeadCell uses a truthy check (if (cell)) on a value typed as React.ReactNode | undefined. This can incorrectly fall through when the renderer returns a legitimate falsy ReactNode (e.g. 0, false, ''), causing the fallback header to render instead. Fix by checking cell !== undefined && cell !== null (or cell == null guard) rather than truthiness.
| const columnKey = String(column.key); | ||
| if (!tableData?.meta) { | ||
| return dataRow[column.key]; | ||
| } |
There was a problem hiding this comment.
TS2 rule: renderDefaultBodyCell uses a truthy check (if (cell)) on a value typed as React.ReactNode | undefined. This can incorrectly fall through for falsy-but-valid ReactNodes, rendering the fallback field renderer instead of the custom renderer. Fix by using an explicit null/undefined check (cell !== undefined && cell !== null).
| @@ -80,7 +80,7 @@ export function getAggregateFieldsFromLocation( | |||
| } else if (isBaseVisualize(groupByOrBaseVisualize)) { | |||
There was a problem hiding this comment.
Correctness bug: updateLocationWithAggregateFields now uses flatMap and returns visualizes.map(...) for isBaseVisualize, but Visualize.fromJSON(aggregateField) returns Visualize[] (per diff). If aggregateField is already a BaseVisualize instance (not JSON), this conversion may be wrong and could change semantics of what gets serialized into location.query.aggregateField. Fix by ensuring aggregateField is the expected JSON shape before calling fromJSON, or by serializing directly from the instance.
| chartType: this.selectedChartType, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential logic bug: Visualize constructor changed from accepting yAxes: readonly string[] to yAxis: string, and it now sets this.stack = 'all' unconditionally. Previously stack was undefined for multi-yAxis bar charts (this.chartType === BAR && this.yAxes.length > 1). This can change chart rendering behavior (stacking) for cases that previously relied on multi-axis semantics. Fix by reintroducing the prior stack logic based on the number of y axes (or by updating downstream expectations).
There was a problem hiding this comment.
🛡️ re-entry.ai Code Review
@CodingKylo — Please review the following assessment:
🚨 Risk Score: 95/100 · CRITICAL
Intent
Implement a self-serve replay bulk delete flow and extend related replay breadcrumb/error-context and browser reporting ingestion, alongside new gated UI for AI feedback summaries.
Summary
This change introduces backend behavior updates for replay breadcrumb summarization (optional error-context enrichment via Snuba/nodestore) and switches the browser reporting collector endpoint from a dataclass parser to a DRF serializer with structured 422 responses. It also adds a new frontend “Feedback Summary” component/hook gated by an org feature flag. The highest risks to verify are (1) correctness/contract stability of the new endpoint response shapes under edge cases (no error IDs, enable_error_context=false, and validation failures) and (2) runtime safety in the new React feature gating (feature flag checks and null/undefined handling). Additionally, the diff shows multiple TypeScript correctness warnings around falsy ReactNode handling and visualization contract changes that can cause subtle UI regressions.
🎯 Review Focus
Verify runtime safety and contract stability in the new/changed endpoints and UI gating: specifically organization.features.includes(...) in FeedbackSummary and the replay breadcrumbs/browser reporting collector response shapes under edge cases (no error IDs, enable_error_context=false, and DRF 422 validation failures).
Key Findings
- 🚨 [static/app/components/feedback/feedbackSummary.tsx:L14] CRITICAL:
organization.features.includes(...)can throw iforganization.featuresisundefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;).
✅ Action Checklist
- CRITICAL [static/app/components/feedback/feedbackSummary.tsx:L14] —
organization.features.includes(...)can throw iforganization.featuresisundefined; fix by guarding with optional chaining or defaulting to an empty array (e.g.,const features = organization.features ?? []; if (!features.includes(...)) return null;). - SUGGESTION — Add a regression test for
FeedbackSummarywhenorganization.featuresis missing/undefined to ensure it returns null instead of crashing:static/app/components/feedback/feedbackSummary.tsx(around theincludescheck). - SUGGESTION — For
defaultTableCellRenderers.tsx, add unit tests covering falsy-but-valid ReactNodes for both head and body renderers (0/false/''), and assert the custom renderer is used instead of the fallback:static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx. - SUGGESTION — For
browser_reporting_collector.py, add/extend tests to lock the request/response contract for the new DRF serializer path: verify 422 payload structure and cross-field validation (e.g.,ageandtimestampmutually exclusive;attempts < 1rejected) and ensure 200 responses still match the previous schema:src/sentry/issues/endpoints/browser_reporting_collector.py. - SUGGESTION — For
project_replay_summarize_breadcrumbs.py, add tests for response stability whenenable_error_context=falseand when Snuba returns noerror_ids(ensure the merged chronological log/error list and pagination metadata remain consistent):src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py.
📝 This review includes 17 inline comments (17 warnings)
ℹ️ Reviewed the highest-priority findings first — 13 additional findings available in the dashboard.
Posted by re-entry.ai · Risk governance for autonomous engineering teams
| ): React.ReactNode { | ||
| const cell = renderTableHeadCell?.(column, _columnIndex); | ||
| if (cell) { | ||
| return cell; |
There was a problem hiding this comment.
TS2 rule: renderDefaultHeadCell uses a truthy check (if (cell)) on a value typed as React.ReactNode | undefined. This can incorrectly fall through when the renderer returns a legitimate falsy ReactNode (e.g. 0, false, ''), causing the fallback header to render instead. Fix by checking cell !== undefined && cell !== null (or cell == null guard) rather than truthiness.
| const columnKey = String(column.key); | ||
| if (!tableData?.meta) { | ||
| return dataRow[column.key]; | ||
| } |
There was a problem hiding this comment.
TS2 rule: renderDefaultBodyCell uses a truthy check (if (cell)) on a value typed as React.ReactNode | undefined. This can incorrectly fall through for falsy-but-valid ReactNodes, rendering the fallback field renderer instead of the custom renderer. Fix by using an explicit null/undefined check (cell !== undefined && cell !== null).
| @@ -80,7 +80,7 @@ export function getAggregateFieldsFromLocation( | |||
| } else if (isBaseVisualize(groupByOrBaseVisualize)) { | |||
There was a problem hiding this comment.
Correctness bug: updateLocationWithAggregateFields now uses flatMap and returns visualizes.map(...) for isBaseVisualize, but Visualize.fromJSON(aggregateField) returns Visualize[] (per diff). If aggregateField is already a BaseVisualize instance (not JSON), this conversion may be wrong and could change semantics of what gets serialized into location.query.aggregateField. Fix by ensuring aggregateField is the expected JSON shape before calling fromJSON, or by serializing directly from the instance.
| chartType: this.selectedChartType, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential logic bug: Visualize constructor changed from accepting yAxes: readonly string[] to yAxis: string, and it now sets this.stack = 'all' unconditionally. Previously stack was undefined for multi-yAxis bar charts (this.chartType === BAR && this.yAxes.length > 1). This can change chart rendering behavior (stacking) for cases that previously relied on multi-axis semantics. Fix by reintroducing the prior stack logic based on the number of y axes (or by updating downstream expectations).
20 participants
Martian Code Review Benchmark PR (mirrored from source #5)