Replays Self-Serve Bulk Delete System - #1
Open
linxia0415 wants to merge 32 commits into
Open
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.
- getsentry/sentry#93894 removed usage - getsentry/sentry-options-automator#4243 removed the last override
When the max segment ID is null the process fails. We should exit early since if there aren't any segments to delete there's nothing to do.
📝 WalkthroughWalkthroughBackend updates add DRF validation to browser reporting, augment replay breadcrumb summaries with error context, remove regex parameterization, configure task deadlines, and record a preprod analytics event. Frontend refactors Explore to a single yAxis model, introduces TableWidgetVisualization, broad Flex layout updates, and adds a Feedback AI summary. ChangesUnified application and UI changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
static/app/views/performance/newTraceDetails/traceDrawer/details/span/eapSections/attributes.tsx (1)
59-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the hidden-attribute filter before the empty-search early return.
Right now
project_id,received, andis_segmentare still shown on initial render because the!searchQuery.trim()branch returnssortedbeforeHIDDEN_ATTRIBUTESis applied.Suggested fix
const sortedAndFilteredAttributes = useMemo(() => { const sorted = sortAttributes(attributes); + const visible = sorted.filter( + attribute => !HIDDEN_ATTRIBUTES.includes(attribute.name) + ); + if (!searchQuery.trim()) { - return sorted; + return visible; } - return sorted.filter( + return visible.filter( attribute => - !HIDDEN_ATTRIBUTES.includes(attribute.name) && attribute.name.toLowerCase().trim().includes(searchQuery.toLowerCase().trim()) ); }, [attributes, searchQuery]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/app/views/performance/newTraceDetails/traceDrawer/details/span/eapSections/attributes.tsx` around lines 59 - 69, The useMemo computed in sortedAndFilteredAttributes currently returns the full sorted list when searchQuery is empty before excluding HIDDEN_ATTRIBUTES, causing hidden keys (e.g., project_id) to appear on initial render; change the logic inside the useMemo (the block that calls sortAttributes(attributes)) to first filter out HIDDEN_ATTRIBUTES from the sorted array (use the attribute => !HIDDEN_ATTRIBUTES.includes(attribute.name) predicate) and then, if searchQuery is empty, return that filtered list, otherwise further filter that list by the searchQuery match; this touches sortedAndFilteredAttributes, sortAttributes, HIDDEN_ATTRIBUTES, searchQuery, and attributes.static/app/views/explore/contexts/pageParamsContext/aggregateFields.tsx (1)
21-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard these type checks against
null.Both guards can still throw on malformed query state because
typeof null === 'object'. SinceparseGroupByOrBaseVisualizereturnsnull, a badaggregateFieldentry will hitvalue.yAxes/'yAxis' in valueand crash parsing instead of falling back cleanly.Suggested fix
export function isBaseVisualize(value: any): value is BaseVisualize { return ( - typeof value === 'object' && + value !== null && + typeof value === 'object' && Array.isArray(value.yAxes) && value.yAxes.every((v: any) => typeof v === 'string') && (!defined(value.chartType) || Object.values(ChartType).includes(value.chartType)) ); } @@ export function isVisualize(value: any): value is Visualize { - return typeof value === 'object' && 'yAxis' in value && typeof value.yAxis === 'string'; + return ( + value !== null && + typeof value === 'object' && + 'yAxis' in value && + typeof value.yAxis === 'string' + ); }Also applies to: 34-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/app/views/explore/contexts/pageParamsContext/aggregateFields.tsx` around lines 21 - 27, The type guards don't guard against null because typeof null === 'object', so update isBaseVisualize (and the similar guard around lines 34-35) to first check value !== null (or Boolean(value)) before accessing properties; specifically ensure the predicate for isBaseVisualize verifies value is non-null object, then checks Array.isArray(value.yAxes) and the chartType inclusion, and likewise guard the other check that uses 'yAxis' in value to avoid throwing when parseGroupByOrBaseVisualize returns null.static/app/views/explore/contexts/pageParamsContext/index.tsx (1)
370-378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
yAxiswhen computing writable visualize refs.This branch still reads
visualize.yAxes, soderiveUpdatedAutoFieldsmisses the new single-axis payloads emitted by the updated Explore setters. The result is that changing visualizes can stop auto-managed table columns from being inserted or cleaned up.Proposed fix
const writableVisualizeFields = // null means to clear it so make sure to handle it correctly writablePageParams.aggregateFields === null ? [] : writablePageParams.aggregateFields ?.filter<BaseVisualize>(isBaseVisualize) - ?.flatMap(visualize => visualize.yAxes) + ?.map(visualize => visualize.yAxis) ?.map(yAxis => parseFunction(yAxis)?.arguments?.[0]) ?.filter<string>(defined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/app/views/explore/contexts/pageParamsContext/index.tsx` around lines 370 - 378, The branch computing writableVisualizeFields still reads visualize.yAxes so it misses new single-axis payloads; update the extraction in writableVisualizeFields (which uses writablePageParams.aggregateFields and isBaseVisualize) to handle both legacy visualize.yAxes and the new visualize.yAxis (e.g., normalize to an array before flatMap or flatMap over (visualize.yAxes ?? [visualize.yAxis])) so deriveUpdatedAutoFields sees both old and new payload shapes and auto-managed table columns are correctly inserted/cleaned.
🧹 Nitpick comments (3)
tests/snuba/api/endpoints/test_organization_events_stats.py (1)
3150-3189: ⚡ Quick winAdd a top-events equation case for
dataset="ourlogs".This new test only covers
count(), but the production change insrc/sentry/snuba/ourlogs.pyis specifically the newequationsplumbing for top-events. Please add one assertion that uses an equation y-axis so this path is actually covered.Suggested test shape
+ def test_top_events_with_equation(self): + with self.feature(self.enabled_features): + response = self.client.get( + self.url, + data={ + "start": self.day_ago.isoformat(), + "end": (self.day_ago + timedelta(hours=2)).isoformat(), + "dataset": "ourlogs", + "interval": "1h", + "yAxis": "equation|count() / 10", + "orderby": ["-count()"], + "field": ["count()", "message", "equation|count() / 10"], + "topEvents": "5", + }, + format="json", + ) + + assert response.status_code == 200, response.content + assert [{"count": 6.0}] in response.data["twenty five seconds"]["data"][0] + assert [{"count": 1.0}] in response.data["Other"]["data"][0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/snuba/api/endpoints/test_organization_events_stats.py` around lines 3150 - 3189, The test_simple_top_events test covers top-events for dataset="ourlogs" but only uses a plain yAxis "count()", so the new equations plumbing in ourlogs.py isn't exercised; update test_simple_top_events to include one additional request or assertion that uses an equation y-axis (e.g. set yAxis to an equation string and include corresponding field/orderby) so the topEvents path that builds/reads equations is hit—modify the existing request data (or add a second request) and assert the response contains the expected top event buckets and ordering for that equation case just like the current count() assertions.static/app/views/explore/contexts/pageParamsContext/index.spec.tsx (1)
118-119: ⚡ Quick winAdd a setter test that writes the new
{yAxis}shape.These assertions were updated for the single-axis read model, but this suite still drives
setPageParams/setVisualizeswith legacy{yAxes: [...]}payloads. That leaves the new writable contract effectively untested and would not catch regressions in auto-managed field syncing.Also applies to: 545-546, 576-587
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/app/views/explore/contexts/pageParamsContext/index.spec.tsx` around lines 118 - 119, Add a unit test that exercises the new writable yAxis shape: when calling setPageParams or setVisualizes, send the new {yAxis: {...}} payload (not legacy {yAxes: [...]}) and assert the store/state is updated with the exact new yAxis shape and that any auto-managed fields (e.g., label, seriesName, axis assignment) are synchronized as expected; update or add assertions near the existing tests that currently call setPageParams/setVisualizes so they use the single-axis write contract (references: setPageParams, setVisualizes, Visualize) and include a failing-case assertion to catch regressions in auto-managed field syncing.src/sentry/feedback/usecases/feedback_summaries.py (1)
16-20: ⚡ Quick winEnforce the new summary-length contract in code.
These lines turn the 55-word / two-sentence limit into a hard requirement, but the server still returns any model output after whitespace cleanup. A non-compliant response will go straight to the UI, so this should be validated or truncated in
parse_response/generate_summaryinstead of relying on prompt adherence alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/feedback/usecases/feedback_summaries.py` around lines 16 - 20, The prompt enforces a 55-word / two-sentence summary but the server still accepts non‑compliant model output; update the feedback summary pipeline to validate/enforce the contract in code by adding strict checks and truncation in parse_response and/or generate_summary: ensure parse_response counts words and sentences (rejects or trims to 55 words and max 2 sentences), return a deterministic sanitized string, and have generate_summary enforce the same post‑processing fallback so no raw model output reaches the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sentry/integrations/source_code_management/commit_context.py`:
- Around line 577-582: The _truncate_title function currently slices
title[:max_length] then appends "..." which can exceed ISSUE_TITLE_MAX_LENGTH;
change the truncation logic in _truncate_title(title, max_length) to slice to
max_length - len("...") (i.e., max_length - 3) before appending the ellipsis,
and handle edge cases where max_length <= 3 by returning a trimmed substring of
length max_length (no extra dots) or an appropriate short indicator so the
returned string never exceeds max_length.
- Around line 584-606: Environment names are inserted directly into SCM markdown
via get_environment_info and then into get_merged_pr_single_issue_template,
allowing backticks/newlines to break the inline code span or inject
markdown/mentions; sanitize/escape environment.name (e.g., replace backticks
with escaped backticks like \` and collapse newlines to spaces, also escape
backslashes) before returning it from get_environment_info or before formatting
MERGED_PR_SINGLE_ISSUE_TEMPLATE so the value used by
get_merged_pr_single_issue_template is safe; update get_environment_info to
perform this escaping (or call a small sanitizer helper) and ensure code still
uses PRCommentWorkflow._truncate_title and MERGED_PR_SINGLE_ISSUE_TEMPLATE
unchanged except for receiving the sanitized environment string.
In `@src/sentry/issues/endpoints/browser_reporting_collector.py`:
- Around line 111-117: The current logger.warning call leaks attacker-controlled
`report` payloads; remove writing the full `raw_report` and instead log only
safe metadata (e.g., `serializer.errors`, a truncated/hashed summary, or
lengths) when returning the 422 Response in the browser reporting collector.
Update the `logger.warning` invocation (where
`logger.warning("browser_report_validation_failed", extra=...)` is used) to omit
`raw_report` and include only non-sensitive fields (e.g., validation errors and
a short sanitized summary or size/hash) before returning the Response with
`{"error": "Invalid report data", "details": serializer.errors}` and
`HTTP_422_UNPROCESSABLE_ENTITY`.
- Around line 50-59: The validators validate_timestamp and validate_age
currently check self.initial_data.get("age") / get("timestamp") which treats 0
as falsy and misses presence; change these to check key presence (e.g. if "age"
in self.initial_data and if "timestamp" in self.initial_data) so zero values are
detected as present and the exclusivity ValidationError triggers correctly;
update the checks in the validate_timestamp and validate_age methods accordingly
while still returning the validated value.
In `@src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py`:
- Around line 107-120: The list comprehension pairs event_ids with nodestore
results using events.values(), which can misalign payloads because
nodestore.backend.get_multi() returns a dict keyed by node id; instead, for each
event_id in error_ids compute node_id via Event.generate_node_id(project_id,
event_id=event_id), look up payload = events.get(node_id), and only create an
ErrorEvent when payload is not None, using payload.get(...) for
title/timestamp/message; update references to node_ids, Event.generate_node_id,
nodestore.backend.get_multi, and ErrorEvent accordingly to preserve correct
event_id↔payload alignment.
- Around line 67-95: Wrap the Snuba lookup and processing so failures are
best-effort: surround the calls to query_replay_instance(...) and
process_raw_response(...) (the block that computes response and error_ids) with
a try/except that on any exception sets error_events = [] (and optionally logs
the exception) and lets execution continue; keep the existing
disable_error_fetching check and only call fetch_error_details(...) when not
disabled and when error_ids were successfully obtained, otherwise ensure
error_events remains an empty list so breadcrumb summarization proceeds without
hard dependency on the Snuba/enrichment path.
In `@src/sentry/tasks/auth/check_auth.py`:
- Around line 76-78: The batch timeout is too short and can kill
check_auth_identities() mid-loop; either increase the
TaskworkerConfig.processing_deadline_duration for auth_control_tasks to exceed
the worst-case runtime or add checkpointing/rescheduling inside
check_auth()/check_auth_identities(): after processing each auth_identity_id (or
every N ids) persist progress and enqueue the remaining ids (or schedule a
follow-up task) before the deadline so the loop cannot be terminated leaving
identities unprocessed; update the TaskworkerConfig(...) instantiation or add a
checkpoint/reschedule call in check_auth_identities() accordingly.
In `@static/app/components/feedback/list/useFeedbackSummary.tsx`:
- Around line 61-66: The return currently treats non-network responses as
successful; update the return in useFeedbackSummary so that when data.success is
false or data.summary is null/undefined you mark the response as an
error/hidden: set summary to undefined (or null), set isError to true, keep
isPending false, and compute tooFewFeedbacks from data.numFeedbacksUsed only
when data.success is true; modify the block referencing data.summary,
data.success, and data.numFeedbacksUsed to implement this conditional behavior
(so unsuccessful/null-summary responses are not treated as renderable).
In `@static/app/components/scrollCarousel.tsx`:
- Around line 202-205: Replace the literal CSS "transparent" in the right-side
mask with a transparentized version of the theme color so the gradient fades to
theme.background (not transparent black); in the styled component that uses
p.transparentMask and p.theme.background (in scrollCarousel.tsx) change the
gradient from `linear-gradient(to right, transparent, ${p.theme.background})` to
use a transparent theme color like `linear-gradient(to right,
${p.theme.background}00, ${p.theme.background})` or `linear-gradient(to right,
rgba(<theme-rgba>, 0), ${p.theme.background})` (or use your existing color
utility to produce theme background with alpha) so the right mask matches the
left mask's approach.
In `@static/app/views/codecov/tests/onboardingSteps/addUploadToken.tsx`:
- Around line 77-85: The nested Flex containers both use
justify="space-between", causing the two CodeSnippet items to be pushed to the
far edges; update the inner Flex (the one wrapping CodeSnippet and CodeSnippet
with SENTRY_PREVENT_TOKEN and FULL_TOKEN) to use justify="flex-start" or remove
the justify prop so spacing is controlled only by gap, and apply the same change
to the second occurrence so the outer Flex remains justify="space-between"
(positioning the snippet group vs the Button which calls handleDoneClick) while
the inner snippet group stays compact.
In `@static/app/views/dashboards/widgetCard/chart.tsx`:
- Around line 164-173: The feature-flag branch is rendering a placeholder
instead of the real query output; update the
organization.features.includes('use-table-widget-visualization') branch so
TableWidgetVisualization receives the actual query result (e.g., use
result.columns and the result data/meta) instead of columns={[]} and an empty
tableData object—wire TableWidgetVisualization to the current query result
variables (result, result.data, result.columns, result.meta or result.table) so
real table widgets render.
In
`@static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx`:
- Line 81: The code unsafely casts tableData.meta.units?.[columnKey] to string;
instead remove the "as string" cast and handle nullable units explicitly: read
the unit from tableData.meta.units?.[columnKey] as type DataUnit | null (or
string | null), then either (A) pass that nullable value through to
fieldRenderer (and update fieldRenderer's parameter type/signature to accept
string | null) or (B) normalize it before calling fieldRenderer using a safe
default (e.g., '' or undefined) via nullish coalescing; reference the symbols
tableData.meta.units, columnKey, and fieldRenderer when making the change.
In `@static/app/views/explore/hooks/useGetTraceItemAttributeKeys.tsx`:
- Around line 95-100: The filter currently only preserves tag attributes
matching /^tags\[[a-zA-Z0-9_.:-]+,number\]$/, dropping string-typed EAP tags;
update the conditional in useGetTraceItemAttributeKeys (the if that checks
attribute.key) to also allow string-typed tags by expanding the second regex to
accept "string" (or both "number" and "string"), e.g., change the pattern used
for tags[...] to include string type so attribute.key values like
tags[foo,string] pass the allowlist and are not filtered out.
In `@static/app/views/explore/hooks/useTraceItemAttributeKeys.tsx`:
- Around line 50-61: The hook useTraceItemAttributeKeys can return attributes as
undefined during initial fetch or when enabled is false, which breaks callers
expecting a TagCollection; update the return to always provide a concrete
TagCollection by defaulting attributes to an empty object when data and previous
are undefined — e.g. compute attributes = isFetching ? (previous ?? {} as
TagCollection) : (data ?? {} as TagCollection) — referencing useQuery,
getTraceItemAttributeKeys, previous, and TagCollection so downstream callers can
safely call Object.keys/hasOwnProperty.
In `@static/app/views/insights/pages/transactionNameSearchBar.tsx`:
- Around line 54-57: The autocomplete hook call getTraceItemAttributeValues
currently omits the caller-provided project scope; update the
useGetTraceItemAttributeValues invocations (including the one assigned to
getTraceItemAttributeValues and the other occurrence around lines 138-144) to
pass the component's projectIds prop (e.g., projectIds: projectIds) so the hook
queries using the explicit project scope rather than the global page filters;
ensure the hook call includes traceItemType and type as before and only adds
projectIds to preserve parent-provided scope for suggestions.
In `@static/app/views/organizationStats/teamInsights/teamMisery.tsx`:
- Around line 98-100: Replacing FlexCenter with Flex align="center" removed
horizontal centering (justify-content:center) for the header/project/misery
cells; restore horizontal centering by either reverting to FlexCenter or adding
justify="center" to the Flex instances that render the cells (e.g., the Flex
wrapping StyledIconStar and the other Flex at lines ~151-157 that display
header/project/misery content) so those table cells are both vertically and
horizontally centered.
---
Outside diff comments:
In `@static/app/views/explore/contexts/pageParamsContext/aggregateFields.tsx`:
- Around line 21-27: The type guards don't guard against null because typeof
null === 'object', so update isBaseVisualize (and the similar guard around lines
34-35) to first check value !== null (or Boolean(value)) before accessing
properties; specifically ensure the predicate for isBaseVisualize verifies value
is non-null object, then checks Array.isArray(value.yAxes) and the chartType
inclusion, and likewise guard the other check that uses 'yAxis' in value to
avoid throwing when parseGroupByOrBaseVisualize returns null.
In `@static/app/views/explore/contexts/pageParamsContext/index.tsx`:
- Around line 370-378: The branch computing writableVisualizeFields still reads
visualize.yAxes so it misses new single-axis payloads; update the extraction in
writableVisualizeFields (which uses writablePageParams.aggregateFields and
isBaseVisualize) to handle both legacy visualize.yAxes and the new
visualize.yAxis (e.g., normalize to an array before flatMap or flatMap over
(visualize.yAxes ?? [visualize.yAxis])) so deriveUpdatedAutoFields sees both old
and new payload shapes and auto-managed table columns are correctly
inserted/cleaned.
In
`@static/app/views/performance/newTraceDetails/traceDrawer/details/span/eapSections/attributes.tsx`:
- Around line 59-69: The useMemo computed in sortedAndFilteredAttributes
currently returns the full sorted list when searchQuery is empty before
excluding HIDDEN_ATTRIBUTES, causing hidden keys (e.g., project_id) to appear on
initial render; change the logic inside the useMemo (the block that calls
sortAttributes(attributes)) to first filter out HIDDEN_ATTRIBUTES from the
sorted array (use the attribute => !HIDDEN_ATTRIBUTES.includes(attribute.name)
predicate) and then, if searchQuery is empty, return that filtered list,
otherwise further filter that list by the searchQuery match; this touches
sortedAndFilteredAttributes, sortAttributes, HIDDEN_ATTRIBUTES, searchQuery, and
attributes.
---
Nitpick comments:
In `@src/sentry/feedback/usecases/feedback_summaries.py`:
- Around line 16-20: The prompt enforces a 55-word / two-sentence summary but
the server still accepts non‑compliant model output; update the feedback summary
pipeline to validate/enforce the contract in code by adding strict checks and
truncation in parse_response and/or generate_summary: ensure parse_response
counts words and sentences (rejects or trims to 55 words and max 2 sentences),
return a deterministic sanitized string, and have generate_summary enforce the
same post‑processing fallback so no raw model output reaches the UI.
In `@static/app/views/explore/contexts/pageParamsContext/index.spec.tsx`:
- Around line 118-119: Add a unit test that exercises the new writable yAxis
shape: when calling setPageParams or setVisualizes, send the new {yAxis: {...}}
payload (not legacy {yAxes: [...]}) and assert the store/state is updated with
the exact new yAxis shape and that any auto-managed fields (e.g., label,
seriesName, axis assignment) are synchronized as expected; update or add
assertions near the existing tests that currently call
setPageParams/setVisualizes so they use the single-axis write contract
(references: setPageParams, setVisualizes, Visualize) and include a failing-case
assertion to catch regressions in auto-managed field syncing.
In `@tests/snuba/api/endpoints/test_organization_events_stats.py`:
- Around line 3150-3189: The test_simple_top_events test covers top-events for
dataset="ourlogs" but only uses a plain yAxis "count()", so the new equations
plumbing in ourlogs.py isn't exercised; update test_simple_top_events to include
one additional request or assertion that uses an equation y-axis (e.g. set yAxis
to an equation string and include corresponding field/orderby) so the topEvents
path that builds/reads equations is hit—modify the existing request data (or add
a second request) and assert the response contains the expected top event
buckets and ordering for that equation case just like the current count()
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39b0e752-f419-4bf0-8560-6baf53306b57
📒 Files selected for processing (106)
devservices/config.ymlsrc/sentry/constants.pysrc/sentry/feedback/usecases/feedback_summaries.pysrc/sentry/grouping/parameterization.pysrc/sentry/hybridcloud/tasks/deliver_webhooks.pysrc/sentry/integrations/github/integration.pysrc/sentry/integrations/gitlab/integration.pysrc/sentry/integrations/source_code_management/commit_context.pysrc/sentry/issues/endpoints/browser_reporting_collector.pysrc/sentry/issues/grouptype.pysrc/sentry/migrations/0917_convert_org_saved_searches_to_views.pysrc/sentry/migrations/0920_convert_org_saved_searches_to_views_revised.pysrc/sentry/options/defaults.pysrc/sentry/preprod/__init__.pysrc/sentry/preprod/analytics.pysrc/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.pysrc/sentry/projectoptions/defaults.pysrc/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.pysrc/sentry/replays/usecases/delete.pysrc/sentry/snuba/ourlogs.pysrc/sentry/tasks/auth/check_auth.pysrc/sentry/workflow_engine/endpoints/validators/base/detector.pysrc/sentry/workflow_engine/processors/delayed_workflow.pysrc/sentry/workflow_engine/processors/workflow.pystatic/app/components/codeSnippet.tsxstatic/app/components/codecov/branchSelector/branchSelector.tsxstatic/app/components/codecov/datePicker/dateSelector.tsxstatic/app/components/codecov/integratedOrgSelector/integratedOrgSelector.tsxstatic/app/components/codecov/repoPicker/repoSelector.tsxstatic/app/components/core/button/styles.chonk.tsxstatic/app/components/events/eventAttachments.tsxstatic/app/components/events/groupingInfo/groupingVariant.tsxstatic/app/components/events/interfaces/spans/newTraceDetailsHeader.tsxstatic/app/components/feedback/feedbackSummary.tsxstatic/app/components/feedback/list/useFeedbackSummary.tsxstatic/app/components/group/times.tsxstatic/app/components/replays/breadcrumbs/breadcrumbItem.tsxstatic/app/components/replays/timeAndScrubberGrid.tsxstatic/app/components/scrollCarousel.tsxstatic/app/utils/analytics/replayAnalyticsEvents.tsxstatic/app/views/alerts/list/rules/alertRuleStatus.tsxstatic/app/views/alerts/list/rules/row.tsxstatic/app/views/codecov/tests/onboardingSteps/addUploadToken.tsxstatic/app/views/dashboards/widgetCard/chart.tsxstatic/app/views/dashboards/widgets/common/types.tsxstatic/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsxstatic/app/views/dashboards/widgets/tableWidget/fixtures/sampleHTTPRequestTableData.tsstatic/app/views/dashboards/widgets/tableWidget/tableWidgetVisualization.spec.tsxstatic/app/views/dashboards/widgets/tableWidget/tableWidgetVisualization.stories.tsxstatic/app/views/dashboards/widgets/tableWidget/tableWidgetVisualization.tsxstatic/app/views/explore/charts/index.tsxstatic/app/views/explore/components/traceItemSearchQueryBuilder.tsxstatic/app/views/explore/contexts/pageParamsContext/aggregateFields.tsxstatic/app/views/explore/contexts/pageParamsContext/index.spec.tsxstatic/app/views/explore/contexts/pageParamsContext/index.tsxstatic/app/views/explore/contexts/pageParamsContext/sortBys.tsxstatic/app/views/explore/contexts/pageParamsContext/visualizes.spec.tsxstatic/app/views/explore/contexts/pageParamsContext/visualizes.tsxstatic/app/views/explore/hooks/useAddToDashboard.tsxstatic/app/views/explore/hooks/useAnalytics.tsxstatic/app/views/explore/hooks/useExploreAggregatesTable.tsxstatic/app/views/explore/hooks/useExploreTimeseries.tsxstatic/app/views/explore/hooks/useGetTraceItemAttributeKeys.tsxstatic/app/views/explore/hooks/useGetTraceItemAttributeValues.spec.tsxstatic/app/views/explore/hooks/useGetTraceItemAttributeValues.tsxstatic/app/views/explore/hooks/useTopEvents.tsxstatic/app/views/explore/hooks/useTraceItemAttributeKeys.tsxstatic/app/views/explore/spans/spansTab.tsxstatic/app/views/explore/tables/aggregateColumnEditorModal.spec.tsxstatic/app/views/explore/tables/aggregateColumnEditorModal.tsxstatic/app/views/explore/toolbar/index.spec.tsxstatic/app/views/explore/toolbar/toolbarSaveAs.tsxstatic/app/views/explore/toolbar/toolbarSortBy.tsxstatic/app/views/explore/toolbar/toolbarVisualize.tsxstatic/app/views/explore/types.tsxstatic/app/views/explore/utils.spec.tsxstatic/app/views/explore/utils.tsxstatic/app/views/feedback/feedbackListPage.tsxstatic/app/views/insights/common/components/chartActionDropdown.tsxstatic/app/views/insights/pages/transactionNameSearchBar.tsxstatic/app/views/organizationStats/teamInsights/teamMisery.tsxstatic/app/views/performance/newTraceDetails/traceDrawer/details/span/eapSections/attributes.tsxstatic/app/views/performance/newTraceDetails/traceTabsAndVitals.tsxstatic/app/views/performance/newTraceDetails/traceWaterfall.tsxstatic/app/views/profiling/profileSummary/index.tsxstatic/app/views/replays/detail/ai/index.tsxstatic/app/views/settings/dynamicSampling/organizationSampleRateInput.tsxstatic/app/views/settings/organizationAuditLog/auditLogList.tsxstatic/app/views/settings/organizationIntegrations/detailedView/integrationLayout.tsxstatic/app/views/settings/project/projectOwnership/codeOwnerFileTable.tsxstatic/gsAdmin/views/instanceLevelOAuth/instanceLevelOAuthDetails.tsxtests/js/fixtures/tabularColumn.tstests/js/fixtures/tabularColumns.tstests/sentry/api/endpoints/test_browser_reporting_collector.pytests/sentry/api/endpoints/test_project_details.pytests/sentry/api/serializers/test_project.pytests/sentry/grouping/test_parameterization.pytests/sentry/integrations/github/tasks/test_pr_comment.pytests/sentry/integrations/gitlab/tasks/test_pr_comment.pytests/sentry/migrations/test_0917_convert_org_saved_searches_to_views.pytests/sentry/replays/tasks/test_delete_replays_bulk.pytests/sentry/replays/test_project_replay_summarize_breadcrumbs.pytests/sentry/workflow_engine/endpoints/test_organization_detector_details.pytests/sentry/workflow_engine/endpoints/test_organization_detector_index.pytests/sentry/workflow_engine/processors/test_delayed_workflow.pytests/snuba/api/endpoints/test_organization_events_stats.py
💤 Files with no reviewable changes (2)
- tests/sentry/migrations/test_0917_convert_org_saved_searches_to_views.py
- src/sentry/options/defaults.py
Comment on lines
+577
to
+582
| @staticmethod | ||
| def _truncate_title(title: str, max_length: int = ISSUE_TITLE_MAX_LENGTH) -> str: | ||
| """Truncate title if it's too long and add ellipsis.""" | ||
| if len(title) <= max_length: | ||
| return title | ||
| return title[:max_length].rstrip() + "..." |
There was a problem hiding this comment.
Keep the truncated title within the configured limit.
Line 582 slices to max_length and then appends ..., so a 50-character cap still emits 53 characters. If this limit is meant to bound the rendered title, trim to max_length - 3 before adding the ellipsis.
Proposed fix
def _truncate_title(title: str, max_length: int = ISSUE_TITLE_MAX_LENGTH) -> str:
"""Truncate title if it's too long and add ellipsis."""
if len(title) <= max_length:
return title
- return title[:max_length].rstrip() + "..."
+ if max_length <= 3:
+ return "." * max_length
+ return title[: max_length - 3].rstrip() + "..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/integrations/source_code_management/commit_context.py` around
lines 577 - 582, The _truncate_title function currently slices
title[:max_length] then appends "..." which can exceed ISSUE_TITLE_MAX_LENGTH;
change the truncation logic in _truncate_title(title, max_length) to slice to
max_length - len("...") (i.e., max_length - 3) before appending the ellipsis,
and handle edge cases where max_length <= 3 by returning a trimmed substring of
length max_length (no extra dots) or an appropriate short indicator so the
returned string never exceeds max_length.
Comment on lines
+584
to
+606
| def get_environment_info(self, issue: Group) -> str: | ||
| try: | ||
| recommended_event = issue.get_recommended_event() | ||
| if recommended_event: | ||
| environment = recommended_event.get_environment() | ||
| if environment and environment.name: | ||
| return f" in `{environment.name}`" | ||
| except Exception as e: | ||
| # If anything goes wrong, just continue without environment info | ||
| logger.info( | ||
| "get_environment_info.no-environment", | ||
| extra={"issue_id": issue.id, "error": e}, | ||
| ) | ||
| return "" | ||
|
|
||
| @staticmethod | ||
| def get_merged_pr_single_issue_template(title: str, url: str, environment: str) -> str: | ||
| truncated_title = PRCommentWorkflow._truncate_title(title) | ||
| return MERGED_PR_SINGLE_ISSUE_TEMPLATE.format( | ||
| title=truncated_title, | ||
| url=url, | ||
| environment=environment, | ||
| ) |
There was a problem hiding this comment.
Escape environment names before inserting them into SCM markdown.
environment.name comes from event data and is interpolated directly into the GitHub/GitLab comment body. A name containing backticks or newlines can break out of the code span and inject arbitrary markdown or mentions into the bot comment.
Proposed fix
def get_environment_info(self, issue: Group) -> str:
try:
recommended_event = issue.get_recommended_event()
if recommended_event:
environment = recommended_event.get_environment()
if environment and environment.name:
- return f" in `{environment.name}`"
+ safe_name = (
+ environment.name.replace("`", "\\`")
+ .replace("\r", " ")
+ .replace("\n", " ")
+ )
+ return f" in `{safe_name}`"
except Exception as e:
# If anything goes wrong, just continue without environment info
logger.info(
"get_environment_info.no-environment",
extra={"issue_id": issue.id, "error": e},🧰 Tools
🪛 Ruff (0.15.15)
[warning] 591-591: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/integrations/source_code_management/commit_context.py` around
lines 584 - 606, Environment names are inserted directly into SCM markdown via
get_environment_info and then into get_merged_pr_single_issue_template, allowing
backticks/newlines to break the inline code span or inject markdown/mentions;
sanitize/escape environment.name (e.g., replace backticks with escaped backticks
like \` and collapse newlines to spaces, also escape backslashes) before
returning it from get_environment_info or before formatting
MERGED_PR_SINGLE_ISSUE_TEMPLATE so the value used by
get_merged_pr_single_issue_template is safe; update get_environment_info to
perform this escaping (or call a small sanitizer helper) and ensure code still
uses PRCommentWorkflow._truncate_title and MERGED_PR_SINGLE_ISSUE_TEMPLATE
unchanged except for receiving the sanitized environment string.
Comment on lines
+50
to
+59
| def validate_timestamp(self, value: int) -> int: | ||
| """Validate that age is absent, but timestamp is present.""" | ||
| if self.initial_data.get("age"): | ||
| raise serializers.ValidationError("If timestamp is present, age must be absent") | ||
| return value | ||
|
|
||
| def validate_age(self, value: int) -> int: | ||
| """Validate that age is present, but not timestamp.""" | ||
| if self.initial_data.get("timestamp"): | ||
| raise serializers.ValidationError("If age is present, timestamp must be absent") |
There was a problem hiding this comment.
Check key presence, not truthiness, for age/timestamp exclusivity.
Line 52 and Line 58 use self.initial_data.get(...), so age=0 or timestamp=0 bypass the mixed-draft guard because 0 is falsy. That lets a payload containing both fields validate successfully.
Proposed fix
def validate_timestamp(self, value: int) -> int:
"""Validate that age is absent, but timestamp is present."""
- if self.initial_data.get("age"):
+ if "age" in self.initial_data:
raise serializers.ValidationError("If timestamp is present, age must be absent")
return value
def validate_age(self, value: int) -> int:
"""Validate that age is present, but not timestamp."""
- if self.initial_data.get("timestamp"):
+ if "timestamp" in self.initial_data:
raise serializers.ValidationError("If age is present, timestamp must be absent")
return value📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def validate_timestamp(self, value: int) -> int: | |
| """Validate that age is absent, but timestamp is present.""" | |
| if self.initial_data.get("age"): | |
| raise serializers.ValidationError("If timestamp is present, age must be absent") | |
| return value | |
| def validate_age(self, value: int) -> int: | |
| """Validate that age is present, but not timestamp.""" | |
| if self.initial_data.get("timestamp"): | |
| raise serializers.ValidationError("If age is present, timestamp must be absent") | |
| def validate_timestamp(self, value: int) -> int: | |
| """Validate that age is absent, but timestamp is present.""" | |
| if "age" in self.initial_data: | |
| raise serializers.ValidationError("If timestamp is present, age must be absent") | |
| return value | |
| def validate_age(self, value: int) -> int: | |
| """Validate that age is present, but not timestamp.""" | |
| if "timestamp" in self.initial_data: | |
| raise serializers.ValidationError("If age is present, timestamp must be absent") | |
| return value |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/issues/endpoints/browser_reporting_collector.py` around lines 50 -
59, The validators validate_timestamp and validate_age currently check
self.initial_data.get("age") / get("timestamp") which treats 0 as falsy and
misses presence; change these to check key presence (e.g. if "age" in
self.initial_data and if "timestamp" in self.initial_data) so zero values are
detected as present and the exclusivity ValidationError triggers correctly;
update the checks in the validate_timestamp and validate_age methods accordingly
while still returning the validated value.
Comment on lines
+111
to
+117
| logger.warning( | ||
| "browser_report_validation_failed", | ||
| extra={"validation_errors": serializer.errors, "raw_report": report}, | ||
| ) | ||
| return Response( | ||
| {"error": "Invalid report data", "details": serializer.errors}, | ||
| status=HTTP_422_UNPROCESSABLE_ENTITY, |
There was a problem hiding this comment.
Avoid logging the full rejected payload here.
This endpoint is unauthenticated and CORS-enabled, so raw_report is entirely attacker-controlled. Writing the full report into logs on every 422 can leak sensitive URL/body data and enables log stuffing with arbitrarily large payloads.
Proposed fix
logger.warning(
"browser_report_validation_failed",
- extra={"validation_errors": serializer.errors, "raw_report": report},
+ extra={
+ "validation_errors": serializer.errors,
+ "report_type": report.get("type") if isinstance(report, dict) else None,
+ "data_type": type(report).__name__,
+ },
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.warning( | |
| "browser_report_validation_failed", | |
| extra={"validation_errors": serializer.errors, "raw_report": report}, | |
| ) | |
| return Response( | |
| {"error": "Invalid report data", "details": serializer.errors}, | |
| status=HTTP_422_UNPROCESSABLE_ENTITY, | |
| logger.warning( | |
| "browser_report_validation_failed", | |
| extra={ | |
| "validation_errors": serializer.errors, | |
| "report_type": report.get("type") if isinstance(report, dict) else None, | |
| "data_type": type(report).__name__, | |
| }, | |
| ) | |
| return Response( | |
| {"error": "Invalid report data", "details": serializer.errors}, | |
| status=HTTP_422_UNPROCESSABLE_ENTITY, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/issues/endpoints/browser_reporting_collector.py` around lines 111
- 117, The current logger.warning call leaks attacker-controlled `report`
payloads; remove writing the full `raw_report` and instead log only safe
metadata (e.g., `serializer.errors`, a truncated/hashed summary, or lengths)
when returning the 422 Response in the browser reporting collector. Update the
`logger.warning` invocation (where
`logger.warning("browser_report_validation_failed", extra=...)` is used) to omit
`raw_report` and include only non-sensitive fields (e.g., validation errors and
a short sanitized summary or size/hash) before returning the Response with
`{"error": "Invalid report data", "details": serializer.errors}` and
`HTTP_422_UNPROCESSABLE_ENTITY`.
Comment on lines
+67
to
+95
| filter_params = self.get_filter_params(request, project) | ||
|
|
||
| # Fetch the replay's error IDs from the replay_id. | ||
| snuba_response = query_replay_instance( | ||
| project_id=project.id, | ||
| replay_id=replay_id, | ||
| start=filter_params["start"], | ||
| end=filter_params["end"], | ||
| organization=project.organization, | ||
| request_user_id=request.user.id, | ||
| ) | ||
|
|
||
| response = process_raw_response( | ||
| snuba_response, | ||
| fields=request.query_params.getlist("field"), | ||
| ) | ||
|
|
||
| error_ids = response[0].get("error_ids", []) if response else [] | ||
|
|
||
| # Check if error fetching should be disabled | ||
| disable_error_fetching = ( | ||
| request.query_params.get("enable_error_context", "true").lower() == "false" | ||
| ) | ||
|
|
||
| if disable_error_fetching: | ||
| error_events = [] | ||
| else: | ||
| error_events = fetch_error_details(project_id=project.id, error_ids=error_ids) | ||
|
|
There was a problem hiding this comment.
Make error-context lookup best-effort.
This new Snuba lookup runs before pagination and is not guarded. If query_replay_instance() or process_raw_response() fails, the whole summary endpoint now fails even though breadcrumb summarization can still proceed without error context. Please catch failures here and fall back to error_events = [] so the additive enrichment path does not become a hard dependency.
Suggested fallback
- # Fetch the replay's error IDs from the replay_id.
- snuba_response = query_replay_instance(
- project_id=project.id,
- replay_id=replay_id,
- start=filter_params["start"],
- end=filter_params["end"],
- organization=project.organization,
- request_user_id=request.user.id,
- )
-
- response = process_raw_response(
- snuba_response,
- fields=request.query_params.getlist("field"),
- )
-
- error_ids = response[0].get("error_ids", []) if response else []
-
- # Check if error fetching should be disabled
+ # Check if error fetching should be disabled
disable_error_fetching = (
request.query_params.get("enable_error_context", "true").lower() == "false"
)
- if disable_error_fetching:
- error_events = []
- else:
- error_events = fetch_error_details(project_id=project.id, error_ids=error_ids)
+ error_events = []
+ if not disable_error_fetching:
+ try:
+ snuba_response = query_replay_instance(
+ project_id=project.id,
+ replay_id=replay_id,
+ start=filter_params["start"],
+ end=filter_params["end"],
+ organization=project.organization,
+ request_user_id=request.user.id,
+ )
+
+ response = process_raw_response(
+ snuba_response,
+ fields=request.query_params.getlist("field"),
+ )
+ error_ids = response[0].get("error_ids", []) if response else []
+ error_events = fetch_error_details(project_id=project.id, error_ids=error_ids)
+ except Exception as exc:
+ sentry_sdk.capture_exception(exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| filter_params = self.get_filter_params(request, project) | |
| # Fetch the replay's error IDs from the replay_id. | |
| snuba_response = query_replay_instance( | |
| project_id=project.id, | |
| replay_id=replay_id, | |
| start=filter_params["start"], | |
| end=filter_params["end"], | |
| organization=project.organization, | |
| request_user_id=request.user.id, | |
| ) | |
| response = process_raw_response( | |
| snuba_response, | |
| fields=request.query_params.getlist("field"), | |
| ) | |
| error_ids = response[0].get("error_ids", []) if response else [] | |
| # Check if error fetching should be disabled | |
| disable_error_fetching = ( | |
| request.query_params.get("enable_error_context", "true").lower() == "false" | |
| ) | |
| if disable_error_fetching: | |
| error_events = [] | |
| else: | |
| error_events = fetch_error_details(project_id=project.id, error_ids=error_ids) | |
| filter_params = self.get_filter_params(request, project) | |
| # Check if error fetching should be disabled | |
| disable_error_fetching = ( | |
| request.query_params.get("enable_error_context", "true").lower() == "false" | |
| ) | |
| error_events = [] | |
| if not disable_error_fetching: | |
| try: | |
| snuba_response = query_replay_instance( | |
| project_id=project.id, | |
| replay_id=replay_id, | |
| start=filter_params["start"], | |
| end=filter_params["end"], | |
| organization=project.organization, | |
| request_user_id=request.user.id, | |
| ) | |
| response = process_raw_response( | |
| snuba_response, | |
| fields=request.query_params.getlist("field"), | |
| ) | |
| error_ids = response[0].get("error_ids", []) if response else [] | |
| error_events = fetch_error_details(project_id=project.id, error_ids=error_ids) | |
| except Exception as exc: | |
| sentry_sdk.capture_exception(exc) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py` around
lines 67 - 95, Wrap the Snuba lookup and processing so failures are best-effort:
surround the calls to query_replay_instance(...) and process_raw_response(...)
(the block that computes response and error_ids) with a try/except that on any
exception sets error_events = [] (and optionally logs the exception) and lets
execution continue; keep the existing disable_error_fetching check and only call
fetch_error_details(...) when not disabled and when error_ids were successfully
obtained, otherwise ensure error_events remains an empty list so breadcrumb
summarization proceeds without hard dependency on the Snuba/enrichment path.
| } | ||
|
|
||
| const fieldRenderer = getFieldRenderer(columnKey, tableData.meta.fields, false); | ||
| const unit = tableData.meta.units?.[columnKey] as string; |
There was a problem hiding this comment.
Unsafe type assertion: unit may be null.
The cast tableData.meta.units?.[columnKey] as string is unsafe. According to the TabularValueUnit type definition (line 60 in types.tsx), units can be DataUnit | null. The fieldRenderer at line 85 receives this unit parameter, and if it expects a string but receives null, it could cause unexpected behavior.
🛡️ Proposed fix to handle null units safely
- const unit = tableData.meta.units?.[columnKey] as string;
+ const unit = tableData.meta.units?.[columnKey] ?? undefined;Or explicitly handle the null case:
- const unit = tableData.meta.units?.[columnKey] as string;
+ const unit = (tableData.meta.units?.[columnKey] as string | null) ?? undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const unit = tableData.meta.units?.[columnKey] as string; | |
| const unit = tableData.meta.units?.[columnKey] ?? undefined; |
Suggested change
| const unit = tableData.meta.units?.[columnKey] as string; | |
| const unit = (tableData.meta.units?.[columnKey] as string | null) ?? undefined; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@static/app/views/dashboards/widgets/tableWidget/defaultTableCellRenderers.tsx`
at line 81, The code unsafely casts tableData.meta.units?.[columnKey] to string;
instead remove the "as string" cast and handle nullable units explicitly: read
the unit from tableData.meta.units?.[columnKey] as type DataUnit | null (or
string | null), then either (A) pass that nullable value through to
fieldRenderer (and update fieldRenderer's parameter type/signature to accept
string | null) or (B) normalize it before calling fieldRenderer using a safe
default (e.g., '' or undefined) via nullish coalescing; reference the symbols
tableData.meta.units, columnKey, and fieldRenderer when making the change.
Comment on lines
+95
to
+100
| // EAP spans contain tags with illegal characters | ||
| // SnQL forbids `-` but is allowed in RPC. So add it back later | ||
| if ( | ||
| !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) && | ||
| !/^tags\[[a-zA-Z0-9_.:-]+,number\]$/.test(attribute.key) | ||
| ) { |
There was a problem hiding this comment.
Allow string tags[...] attributes through this filter.
This allowlist only preserves tags[...,number], so string-typed EAP tag keys such as tags[foo,string] get filtered out entirely. That will make string attribute autocomplete incomplete for trace-item search/builders.
Suggested fix
- if (
- !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) &&
- !/^tags\[[a-zA-Z0-9_.:-]+,number\]$/.test(attribute.key)
- ) {
+ if (
+ !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) &&
+ !/^tags\[[a-zA-Z0-9_.:-]+,(string|number)\]$/.test(attribute.key)
+ ) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // EAP spans contain tags with illegal characters | |
| // SnQL forbids `-` but is allowed in RPC. So add it back later | |
| if ( | |
| !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) && | |
| !/^tags\[[a-zA-Z0-9_.:-]+,number\]$/.test(attribute.key) | |
| ) { | |
| // EAP spans contain tags with illegal characters | |
| // SnQL forbids `-` but is allowed in RPC. So add it back later | |
| if ( | |
| !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) && | |
| !/^tags\[[a-zA-Z0-9_.:-]+,(string|number)\]$/.test(attribute.key) | |
| ) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/app/views/explore/hooks/useGetTraceItemAttributeKeys.tsx` around lines
95 - 100, The filter currently only preserves tag attributes matching
/^tags\[[a-zA-Z0-9_.:-]+,number\]$/, dropping string-typed EAP tags; update the
conditional in useGetTraceItemAttributeKeys (the if that checks attribute.key)
to also allow string-typed tags by expanding the second regex to accept "string"
(or both "number" and "string"), e.g., change the pattern used for tags[...] to
include string type so attribute.key values like tags[foo,string] pass the
allowlist and are not filtered out.
Comment on lines
+50
to
+61
| const {data, isFetching, error} = useQuery<TagCollection>({ | ||
| enabled, | ||
| staleTime: 0, | ||
| refetchOnWindowFocus: false, | ||
| retry: false, | ||
| queryKey, | ||
| queryFn: () => getTraceItemAttributeKeys(), | ||
| }); | ||
|
|
||
| const attributes: TagCollection = useMemo(() => { | ||
| const allAttributes: TagCollection = {}; | ||
|
|
||
| for (const attribute of result.data ?? []) { | ||
| if (isKnownAttribute(attribute)) { | ||
| continue; | ||
| } | ||
|
|
||
| // EAP spans contain tags with illegal characters | ||
| // SnQL forbids `-` but is allowed in RPC. So add it back later | ||
| if ( | ||
| !/^[a-zA-Z0-9_.:-]+$/.test(attribute.key) && | ||
| !/^tags\[[a-zA-Z0-9_.:-]+,number\]$/.test(attribute.key) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| allAttributes[attribute.key] = { | ||
| key: attribute.key, | ||
| name: attribute.name, | ||
| kind: type === 'number' ? FieldKind.MEASUREMENT : FieldKind.TAG, | ||
| }; | ||
| } | ||
|
|
||
| return allAttributes; | ||
| }, [result.data, type]); | ||
|
|
||
| const previousAttributes = usePrevious(attributes, result.isLoading); | ||
| const previous = usePrevious(data, isFetching); | ||
|
|
||
| return { | ||
| attributes: result.isLoading ? previousAttributes : attributes, | ||
| isLoading: result.isLoading, | ||
| attributes: isFetching ? previous : data, | ||
| error, | ||
| isLoading: isFetching, |
There was a problem hiding this comment.
Default attributes to an empty TagCollection.
This hook can now return attributes: undefined on the initial fetch or whenever enabled is false. Downstream Explore code treats this as a concrete object and calls methods like hasOwnProperty/Object.keys, so this can crash before the query resolves.
Suggested fix
const {data, isFetching, error} = useQuery<TagCollection>({
enabled,
queryKey,
queryFn: () => getTraceItemAttributeKeys(),
});
const previous = usePrevious(data, isFetching);
+ const attributes = (isFetching ? previous : data) ?? {};
return {
- attributes: isFetching ? previous : data,
+ attributes,
error,
isLoading: isFetching,
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/app/views/explore/hooks/useTraceItemAttributeKeys.tsx` around lines 50
- 61, The hook useTraceItemAttributeKeys can return attributes as undefined
during initial fetch or when enabled is false, which breaks callers expecting a
TagCollection; update the return to always provide a concrete TagCollection by
defaulting attributes to an empty object when data and previous are undefined —
e.g. compute attributes = isFetching ? (previous ?? {} as TagCollection) : (data
?? {} as TagCollection) — referencing useQuery, getTraceItemAttributeKeys,
previous, and TagCollection so downstream callers can safely call
Object.keys/hasOwnProperty.
Comment on lines
+54
to
57
| const getTraceItemAttributeValues = useGetTraceItemAttributeValues({ | ||
| traceItemType: TraceItemDataset.SPANS, | ||
| attributeKey: 'transaction', | ||
| enabled: true, | ||
| type: 'string', | ||
| }); |
There was a problem hiding this comment.
Preserve the caller-provided project scope for suggestions.
projectIds is still a required prop for this search bar, but the new trace-item attribute fetch no longer uses it. Autocomplete will now query whatever is in page filters instead of the explicit project scope the parent passed in, so suggestions can be wrong while navigation still uses projectIds.
Also applies to: 138-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/app/views/insights/pages/transactionNameSearchBar.tsx` around lines 54
- 57, The autocomplete hook call getTraceItemAttributeValues currently omits the
caller-provided project scope; update the useGetTraceItemAttributeValues
invocations (including the one assigned to getTraceItemAttributeValues and the
other occurrence around lines 138-144) to pass the component's projectIds prop
(e.g., projectIds: projectIds) so the hook queries using the explicit project
scope rather than the global page filters; ensure the hook call includes
traceItemType and type as before and only adds projectIds to preserve
parent-provided scope for suggestions.
Comment on lines
+98
to
+100
| <Flex align="center" key="transaction"> | ||
| <StyledIconStar isSolid color="yellow300" /> {t('Key transaction')} | ||
| </FlexCenter>, | ||
| </Flex>, |
There was a problem hiding this comment.
Restore horizontal centering in these table cells.
Switching from FlexCenter to Flex align="center" drops justify-content: center, so the header/project/misery cells will no longer be centered within their columns.
Suggested fix
- <Flex align="center" key="transaction">
+ <Flex align="center" justify="center" key="transaction">
<StyledIconStar isSolid color="yellow300" /> {t('Key transaction')}
</Flex>
...
- <Flex align="center">
+ <Flex align="center" justify="center">
<ProjectBadgeContainer>
{project && <ProjectBadge avatarSize={18} project={project} />}
</ProjectBadgeContainer>
</Flex>
- <Flex align="center">{periodMisery}</Flex>
- <Flex align="center">{weekMisery ?? '\u2014'}</Flex>
+ <Flex align="center" justify="center">{periodMisery}</Flex>
+ <Flex align="center" justify="center">{weekMisery ?? '\u2014'}</Flex>Also applies to: 151-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/app/views/organizationStats/teamInsights/teamMisery.tsx` around lines
98 - 100, Replacing FlexCenter with Flex align="center" removed horizontal
centering (justify-content:center) for the header/project/misery cells; restore
horizontal centering by either reverting to FlexCenter or adding
justify="center" to the Flex instances that render the cells (e.g., the Flex
wrapping StyledIconStar and the other Flex at lines ~151-157 that display
header/project/misery content) so those table cells are both vertically and
horizontally centered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 5
Summary by CodeRabbit
Release Notes
New Features
Improvements
Style