Repository navigation
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive set of changes across the Sentry codebase focused on system improvements and feature enhancements. The most significant changes include:
Autofix Configuration Changes: The default value for sentry:autofix_automation_tuning has been changed from "low" to "off" across multiple configuration files (src/sentry/constants.py, src/sentry/projectoptions/defaults.py) and corresponding test updates. This makes AI-powered autofix functionality opt-in rather than enabled by default.
Workflow Engine Enhancements: Multiple improvements to the workflow engine including the addition of owner field support for detectors (allowing user/team ownership assignment), performance optimizations in delayed workflow processing by pre-computing slow conditions, and enhanced context-aware logging throughout the system.
Integration Improvements: Updates to GitHub and GitLab PR comment workflows with cleaner formatting, environment information display, and standardized template handling. The changes move from hardcoded templates to method-based approaches for better flexibility.
Replay System Enhancements: Significant improvements to the replay system including bulk deletion bug fixes (handling null max_segment_id values), enhanced breadcrumb summarization with error context integration, and comprehensive test coverage additions.
Analytics and Monitoring: New analytics tracking for preprod artifact assembly operations and various task timeout configurations to improve system reliability.
Code Quality Improvements: Removal of experimental grouping parameterization code, migration cleanup, and extensive test coverage additions across multiple endpoints and workflows.
These changes collectively improve system stability, user experience, and maintainability while adding new functionality for replay management and workflow ownership.
Confidence score: 4/5
• This PR is generally safe to merge with mostly configuration changes, bug fixes, and feature enhancements that follow established patterns.
• The score reflects some potential concerns around the fail-fast validation in browser reporting, hardcoded timeout values, and the significant scope of changes across many system components.
• Files that need more attention: src/sentry/issues/endpoints/browser_reporting_collector.py (fail-fast validation behavior), src/sentry/hybridcloud/tasks/deliver_webhooks.py (timeout values), and src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py (performance impact of error fetching).
39 files reviewed, 5 comments
| @@ -60,6 +62,22 @@ def data_conditions(self) -> BaseDataConditionValidator: | |||
| def update(self, instance: Detector, validated_data: dict[str, Any]): | |||
| instance.name = validated_data.get("name", instance.name) | |||
| instance.type = validated_data.get("detector_type", instance.group_type).slug | |||
There was a problem hiding this comment.
logic: Bug: Using 'detector_type' key but field is named 'type'. Should be validated_data.get('type', instance.group_type).slug
| instance.type = validated_data.get("detector_type", instance.group_type).slug | |
| instance.type = validated_data.get("type", instance.group_type).slug |
|
|
||
| @staticmethod | ||
| def get_merged_pr_single_issue_template(title: str, url: str, environment: str) -> str: | ||
| truncated_title = PRCommentWorkflow._truncate_title(title) |
There was a problem hiding this comment.
logic: The method calls PRCommentWorkflow._truncate_title(title) but could use the passed max_length parameter from the method signature for consistency
| 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") | ||
| return value |
There was a problem hiding this comment.
logic: The mutual exclusion validation between age and timestamp correctly handles the two W3C specification versions, but both fields are optional which means reports with neither field would pass validation. Consider if at least one should be required.
| for report in raw_data: | ||
| browser_report = BrowserReport(**report) | ||
| serializer = BrowserReportSerializer(data=report) | ||
| if not serializer.is_valid(): | ||
| 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.
style: The fail-fast validation approach rejects entire batches if any single report is invalid. Consider if partial success (processing valid reports while logging invalid ones) would be more resilient for browser reporting scenarios.
| iterator: Iterator[tuple[int, memoryview]], error_events: list[ErrorEvent] | ||
| ) -> list[str]: | ||
| # Sort error events by timestamp | ||
| error_events.sort(key=lambda x: x["timestamp"]) |
There was a problem hiding this comment.
style: In-place sorting of error_events could cause issues if the same list is reused elsewhere. Consider using sorted() to create a new list.
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 30 files. 0 high-severity issues found. Verdict: approved.
src/sentry/integrations/gitlab/integration.py (low)
- Reviewed src/sentry/integrations/gitlab/integration.py — looks good
src/sentry/feedback/usecases/feedback_summaries.py (low)
- Reviewed src/sentry/feedback/usecases/feedback_summaries.py — looks good
src/sentry/constants.py (low)
- Reviewed src/sentry/constants.py — looks good
src/sentry/integrations/github/integration.py (low)
- Reviewed src/sentry/integrations/github/integration.py — looks good
src/sentry/hybridcloud/tasks/deliver_webhooks.py (low)
- Reviewed src/sentry/hybridcloud/tasks/deliver_webhooks.py — looks good
src/sentry/integrations/source_code_management/commit_context.py (low)
- Reviewed src/sentry/integrations/source_code_management/commit_context.py — looks good
src/sentry/issues/endpoints/browser_reporting_collector.py (low)
- Reviewed src/sentry/issues/endpoints/browser_reporting_collector.py — looks good
src/sentry/migrations/0920_convert_org_saved_searches_to_views_revised.py (low)
- Reviewed src/sentry/migrations/0920_convert_org_saved_searches_to_views_revised.py — looks good
src/sentry/grouping/parameterization.py (low)
- Reviewed src/sentry/grouping/parameterization.py — looks good
devservices/config.yml (low)
- Reviewed devservices/config.yml — looks good
src/sentry/options/defaults.py (medium)
- Reviewed src/sentry/options/defaults.py — looks good
src/sentry/issues/grouptype.py (low)
- Reviewed src/sentry/issues/grouptype.py — looks good
src/sentry/migrations/0917_convert_org_saved_searches_to_views.py (low)
- Reviewed src/sentry/migrations/0917_convert_org_saved_searches_to_views.py — looks good
src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py (low)
- Reviewed src/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py — looks good
src/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.py (low)
- Reviewed src/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.py — looks good
src/sentry/preprod/__init__.py (low)
- Reviewed src/sentry/preprod/init.py — looks good
src/sentry/projectoptions/defaults.py (low)
- Reviewed src/sentry/projectoptions/defaults.py — looks good
src/sentry/tasks/auth/check_auth.py (low)
- Reviewed src/sentry/tasks/auth/check_auth.py — looks good
src/sentry/snuba/ourlogs.py (low)
- Reviewed src/sentry/snuba/ourlogs.py — looks good
src/sentry/replays/usecases/delete.py (low)
- Reviewed src/sentry/replays/usecases/delete.py — looks good
src/sentry/workflow_engine/processors/workflow.py (low)
- Reviewed src/sentry/workflow_engine/processors/workflow.py — looks good
src/sentry/preprod/analytics.py (low)
- Reviewed src/sentry/preprod/analytics.py — looks good
static/app/components/codeSnippet.tsx (low)
- Reviewed static/app/components/codeSnippet.tsx — looks good
static/app/components/codecov/branchSelector/branchSelector.tsx (low)
- Reviewed static/app/components/codecov/branchSelector/branchSelector.tsx — looks good
static/app/components/codecov/integratedOrgSelector/integratedOrgSelector.tsx (low)
- Reviewed static/app/components/codecov/integratedOrgSelector/integratedOrgSelector.tsx — looks good
static/app/components/codecov/datePicker/dateSelector.tsx (low)
- Reviewed static/app/components/codecov/datePicker/dateSelector.tsx — looks good
src/sentry/workflow_engine/endpoints/validators/base/detector.py (low)
- Reviewed src/sentry/workflow_engine/endpoints/validators/base/detector.py — looks good
static/app/components/core/button/styles.chonk.tsx (low)
- Reviewed static/app/components/core/button/styles.chonk.tsx — looks good
src/sentry/workflow_engine/processors/delayed_workflow.py (low)
- Reviewed src/sentry/workflow_engine/processors/delayed_workflow.py — looks good
static/app/components/codecov/repoPicker/repoSelector.tsx (low)
- Reviewed static/app/components/codecov/repoPicker/repoSelector.tsx — looks good
zach-source
left a comment
zach-source
left a comment
There was a problem hiding this comment.
Data-wiring bugs and validation logic errors identified.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 5
Problem
This PR bundles many unrelated changes across the Sentry codebase: a Visualize API migration (yAxes: string[] → yAxis: string), detector owner-field support, browser reporting collector validation, replay breadcrumb error-context, replays delete null-segment handling, autofix default "low"→"off", migration 0917→0921 rework, ourlogs top-events stats, webhook delivery deadlines, and a broad Flex/Flex.Item styled-component refactor across ~15 frontend files.
Solution Reviewed
Backend changes add serializer-based validation to the browser reporting collector, error-event enrichment to replay breadcrumb summaries, owner assignment to detectors, and a revised migration chain (0917 and 0920 turned into no-ops with the real work in 0921). Frontend changes migrate the explore Visualize type to a single yAxis and replace ad-hoc styled flex containers with the shared Flex/Flex.Item components.
Summary
The implementation is mostly sound — the migration no-op pattern is correct, the Visualize migration updates call sites consistently, and detector owner tests are thorough. However, there are several real edge-case bugs (truthiness-based validation, zip misalignment with nodestore), a webhook deadline shorter than its internal loop bound, analytics firing before a feature gate, and a couple of visual regressions from the Flex refactor. These should be addressed before merge.
Files Reviewed
src/sentry/issues/endpoints/browser_reporting_collector.py— deeply reviewedsrc/sentry/replays/endpoints/project_replay_summarize_breadcrumbs.py— deeply reviewedsrc/sentry/hybridcloud/tasks/deliver_webhooks.py— deeply reviewedsrc/sentry/preprod/api/endpoints/organization_preprod_artifact_assemble.py— deeply reviewedsrc/sentry/integrations/source_code_management/commit_context.py— deeply reviewedsrc/sentry/grouping/parameterization.py— deeply reviewedsrc/sentry/integrations/github/integration.py— deeply reviewedsrc/sentry/integrations/gitlab/integration.py— deeply reviewedsrc/sentry/migrations/0917_*,0920_*,0921_*— deeply reviewed (migration chain verified)static/gsAdmin/views/instanceLevelOAuth/instanceLevelOAuthDetails.tsx— deeply reviewedstatic/app/components/core/layout/flex.tsx— deeply reviewed (for Flex refactor impact)static/app/views/settings/dynamicSampling/organizationSampleRateInput.tsx— lightly reviewed (refactor verified equivalent)static/app/views/explore/**(Visualize migration) — lightly reviewed (call-site consistency)tests/**— lightly reviewed (coverage checked)devservices/config.yml— lightly reviewed
Verification
python3 -m py_compileon all changed Python source files — passed (all compile)- Migration chain verified: 0917 (no-op) → 0919 → 0920 (no-op) → 0921 (real, with test) — consistent
- Full test/typecheck suite skipped: infeasible on a repo of this size for a 60-file PR
Verdict
Recommend changes before merge — the validation, nodestore zip, and webhook deadline issues are real and should be fixed; the Flex visual regressions and dead code are worth cleaning up.
|
|
||
| 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.
🟡 Mutual-exclusion check uses truthiness, bypassed by value 0. validate_timestamp checks if self.initial_data.get("age"): and validate_age checks if self.initial_data.get("timestamp"):. Both age=0 (valid per the W3C Editor's Draft — a just-generated report) and timestamp=0 (valid per min_value=0) are falsy, so the exclusion check is silently skipped and a report with both fields present passes validation. Use "age" in self.initial_data / "timestamp" in self.initial_data to test presence regardless of value.
| attempts = serializers.IntegerField(min_value=1) | ||
| # Fields that do not overlap between specs | ||
| # We need to support both specs | ||
| age = serializers.IntegerField(required=False) |
There was a problem hiding this comment.
🟡 age field missing min_value=0. timestamp has min_value=0 but age = serializers.IntegerField(required=False) does not. The W3C Editor's Draft defines age as a non-negative integer (ms elapsed since generation). Negative age values pass validation unchallenged. Add min_value=0 for consistency with timestamp.
| return HttpResponse(status=404) | ||
| return Response(status=HTTP_404_NOT_FOUND) | ||
|
|
||
| logger.info("browser_report_received", extra={"request_body": request.data}) |
There was a problem hiding this comment.
🟡 Raw report data logged on an unauthenticated endpoint. logger.info("browser_report_received", extra={"request_body": request.data}) (line 93) and the new logger.warning("browser_report_validation_failed", extra={"raw_report": report}) (line 111) both log the full report including the body field, which can contain arbitrary user-controlled data and PII. This endpoint has permission_classes = (), so any external actor can inject data into logs. The PR removed the test assertion for the logger.info call but left the log statement in place — consider removing/redactinging it and redacting body from the warning log.
| timestamp=data.get("timestamp", 0.0), | ||
| message=data.get("message", ""), | ||
| ) | ||
| for event_id, data in zip(error_ids, events.values()) |
There was a problem hiding this comment.
🟡 zip(error_ids, events.values()) misaligns when nodestore omits missing keys. nodestore.backend.get_multi(node_ids) with the default Django backend returns only found keys, so events.values() can have fewer entries than error_ids. zip then pairs the 2nd error_id with the 3rd event's title/message/timestamp, producing incorrect error context in the AI summary. Iterate over error_ids and look up each in the events dict by its node_id key instead of zipping positionally.
| # Check if we need to yield any error messages that occurred before this event | ||
| while error_idx < len(error_events) and error_events[error_idx][ | ||
| "timestamp" | ||
| ] < event.get("timestamp", 0): |
There was a problem hiding this comment.
🟡 Timestamp domain mismatch risk in chronological interleaving. gen_request_data compares error-event timestamps (from nodestore, Unix seconds since epoch) against RRWeb segment event timestamps (event.get("timestamp", 0)) using < to interleave them. If RRWeb segment timestamps are in a different domain in production (e.g. relative milliseconds), the ordering will be wrong and degrade the Seer summary quality. The unit test uses consistent units for both but doesn't prove production data matches. Please verify both timestamp sources share the same domain.
| Assembles a preprod artifact (mobile build, etc.) and stores it in the database. | ||
| """ | ||
|
|
||
| analytics.record( |
There was a problem hiding this comment.
🟡 Analytics event fires before the feature-gate check. analytics.record("preprod_artifact.api.assemble", ...) runs at the top of post() before the features.has("organizations:preprod-artifact-assemble") check on line 88. Requests from orgs without the feature return 404 but still increment the assemble analytics event, skewing the metric with denied requests. Move the analytics.record call after the feature check.
|
|
||
|
|
||
| ISSUE_TITLE_MAX_LENGTH = 50 | ||
| MERGED_PR_SINGLE_ISSUE_TEMPLATE = "* ‼️ [**{title}**]({url}){environment}\n" |
There was a problem hiding this comment.
🟡 Issue title embedded in markdown link without escaping. MERGED_PR_SINGLE_ISSUE_TEMPLATE = "* ‼️ [**{title}**]({url}){environment}\n" places the title inside the link text. Issue titles are user-controlled (error messages); a ] in the title breaks the markdown link. The previous template kept the title outside the link (**{title}** … [View Issue]({url})). Consider escaping ]/[ in the title or reverting to the link-external layout.
| </p> | ||
| </ApiForm> | ||
| <FlexDiv> | ||
| <Flex justify="right"> |
There was a problem hiding this comment.
🟡 <Flex justify="right"> drops width: 100%. The old FlexDiv styled div had display: flex; width: 100%; justify-content: right. The Flex component (flex.tsx) does not set width or flex: 1 by default, so the container shrinks to its content width and justify-content: right no longer pushes the Delete button to the right edge of the parent. Add flex={1} or wrap in a full-width container.
| else: | ||
| content = experiment.run(content, _handle_regex_match) | ||
|
|
||
| content = experiment.run(content, _incr_counter) |
There was a problem hiding this comment.
💡 Dead code: orphaned _handle_regex_match (line 335). After ParameterizationRegexExperiment was removed, the _handle_regex_match defined inside parametrize_w_experiments (line 335) is never called — the loop now unconditionally calls experiment.run(content, _incr_counter). The identical function at line 310 inside parametrize_w_regex is still used. Remove the orphaned copy at line 335.
| issue_list = "\n".join( | ||
| [ | ||
| MERGED_PR_SINGLE_ISSUE_TEMPLATE.format( | ||
| self.get_merged_pr_single_issue_template( |
There was a problem hiding this comment.
💡 Dead code: format_comment_subtitle (line 394) no longer called. This call site was changed from self.format_comment_subtitle(...) to self.get_merged_pr_single_issue_template(...), making format_comment_subtitle (defined at line 394) dead code. The same dead method exists in src/sentry/integrations/gitlab/integration.py:240. Both can be removed.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review — PR #5
Reviewed the full diff (~95 files). Focused on logic errors/edge cases, security, test coverage gaps, and code style. Findings below are deduplicated against existing reviews from greptile-apps[bot], mfeuerstein, zach-source, and ron-x5labs.
Summary of new findings
- [Logic]
chart.tsx— emptyTableWidgetVisualization(high): Feature-flagged table widget renders hardcoded empty data, ignoring actual query results. - [Logic]
browser_reporting_collector.py— missing required-one-of validation (medium): Reports with neitheragenortimestamppass validation, violating the W3C spec. - [Edge case]
project_replay_summarize_breadcrumbs.py— nullerror_ids(medium):.get("error_ids", [])does not guard againstNonevalues. - [Test gap]
project_replay_summarize_breadcrumbs.py— untested fallback path (low): The broadexcept Exceptioninfetch_error_detailshas no test coverage. - [Style]
delete.py— typo (low): "segements" → "segments".
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Inline review comments (deduplicated against existing reviews). See summary review above for context.
|
|
||
| def _make_recording_filenames(project_id: int, row: MatchedRow) -> list[str]: | ||
| # Null segment_ids can cause this to fail. If no segments were ingested then we can skip | ||
| # deleting the segements. |
There was a problem hiding this comment.
📝 Typo: "segements" → "segments" in the comment.
| columns={[]} | ||
| tableData={{ | ||
| data: [], | ||
| meta: { | ||
| fields: {}, | ||
| units: {}, | ||
| }, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🔴 Logic: TableWidgetVisualization renders hardcoded empty data. When the use-table-widget-visualization feature flag is enabled, the component is rendered with columns={[]} and tableData={{ data: [], meta: { fields: {}, units: {} } }} — the actual tableResults, result.data, result.meta, fields, and eventView are all ignored. Enabling this flag would show empty tables in all dashboards. This appears to be scaffolding/placeholder code that should not ship behind a feature flag without wiring real data through.
| fields=request.query_params.getlist("field"), | ||
| ) | ||
|
|
||
| error_ids = response[0].get("error_ids", []) if response else [] |
There was a problem hiding this comment.
🟡 Edge case: error_ids may be None. response[0].get("error_ids", []) returns None if the key exists with a null value (the default [] only applies when the key is missing). This would cause a TypeError when iterating for id in error_ids in fetch_error_details. Consider response[0].get("error_ids") or [] for null-safety.
| age = serializers.IntegerField(required=False) | ||
| timestamp = serializers.IntegerField(required=False, min_value=0) |
There was a problem hiding this comment.
🟡 Logic gap: no validation that at least one of age or timestamp is present. Both fields are required=False, so a report with neither passes validation. The W3C Reporting API spec (both Working Draft and Editor's Draft) requires every report to carry either age or timestamp. Consider adding a validate() method that raises if both are absent. No test covers this case.
| def fetch_error_details(project_id: int, error_ids: list[str]) -> list[ErrorEvent]: | ||
| """Fetch error details given error IDs and return a list of ErrorEvent objects.""" | ||
| try: | ||
| node_ids = [Event.generate_node_id(project_id, event_id=id) for id in error_ids] | ||
| events = nodestore.backend.get_multi(node_ids) | ||
|
|
||
| return [ | ||
| ErrorEvent( | ||
| category="error", | ||
| id=event_id, | ||
| title=data.get("title", ""), | ||
| timestamp=data.get("timestamp", 0.0), | ||
| message=data.get("message", ""), | ||
| ) | ||
| for event_id, data in zip(error_ids, events.values()) | ||
| if data is not None | ||
| ] |
There was a problem hiding this comment.
🟡 Test coverage gap: fallback path untested. fetch_error_details catches all exceptions via except Exception and returns [], silently dropping error context. While the broad catch is reasonable (you don't want nodestore failures to break the entire breadcrumb summary), there is no test exercising this fallback — test_get_with_error only covers the happy path. Consider adding a test that patches nodestore.backend.get_multi to raise and asserts the endpoint still returns 200 with breadcrumb-only logs.
20 participants
Test 5