feat: split issue loading and redesign issue page #28083
Conversation
There was a problem hiding this comment.
PR Summary
This PR introduces significant changes to the error tracking system by splitting issue loading into two phases and redesigning the issue page UI. Here's a summary of the key changes:
- Split issue loading to first fetch relational PostgreSQL model data before ClickHouse queries, allowing for better date range optimization based on when issues were first seen
- Redesigned error tracking issue page by removing tabs in favor of a single-page layout with collapsible panels for stacktrace and recording information
- Added new components
Metadata.tsxandEvents.tsxto better organize issue details and event information - Introduced
EventsMode(Latest/Earliest/Recommended/All) for more flexible event viewing options - Updated schema to split
ErrorTrackingIssueinto separate relational and aggregation models for better data organization
The changes improve performance and fix a bug where queries were incorrectly clamped by the previous page's date range.
26 file(s) reviewed, 20 comment(s)
Edit PR Review Bot Settings | Greptile
| function PanelLayout({ className, ...props }: Omit<PanelContainerProps, 'primary'>): JSX.Element { | ||
| return <Container className={clsx(className, 'PanelLayout')} {...props} primary={false} /> | ||
| } |
There was a problem hiding this comment.
logic: PanelLayout always sets primary to false, which seems to make the primary prop in PanelContainerProps redundant since it's never true
| border: 'bottom' | 'top' | ||
| }>): JSX.Element { |
There was a problem hiding this comment.
style: border prop is required but title is optional - consider making border optional with a default value since it's a visual preference
| id: ErrorTrackingIssue['id'], | ||
| data: Partial<Pick<ErrorTrackingIssue, 'assignee' | 'status'>> | ||
| ): Promise<ErrorTrackingIssue> { | ||
| ): Promise<ErrorTrackingRelationalIssue> { | ||
| return await new ApiRequest().errorTrackingIssue(id).update({ data }) |
There was a problem hiding this comment.
style: Return type changed to ErrorTrackingRelationalIssue but data parameter still uses ErrorTrackingIssue - consider using the relational type for consistency
| & .LemonCollapsePanel > .LemonButton:hover:has(.LemonSegmentedButton:hover) { | ||
| background-color: inherit; | ||
| } |
There was a problem hiding this comment.
style: using :has() selector may cause compatibility issues in older browsers
| onSelect={setOrderBy} | ||
| onChange={setOrderBy} |
There was a problem hiding this comment.
style: redundant handlers - both onSelect and onChange are specified with the same function. Only one is needed since they serve the same purpose
| onSelect={setOrderBy} | |
| onChange={setOrderBy} | |
| onChange={setOrderBy} | |
| onChange={setOrderBy} |
| return ( | ||
| <> | ||
| <PanelLayout.PanelSettings title="Events" border="bottom"> | ||
| {eventsMode != EventsMode.All && ( |
There was a problem hiding this comment.
syntax: != operator used instead of !== for strict equality comparison with enum
| {eventsMode != EventsMode.All && ( | |
| {eventsMode !== EventsMode.All && ( |
| )} | ||
| <SettingsButton | ||
| label={eventsMode === EventsMode.All ? 'Close' : 'View all events'} | ||
| active |
There was a problem hiding this comment.
style: active prop is always true, making the button highlight state static
| after: dateRange.date_from || issue.firstSeen, | ||
| before: dateRange.date_to || undefined, |
There was a problem hiding this comment.
logic: Using issue.firstSeen as a fallback for date_from could potentially miss events if the issue was merged from other issues with earlier timestamps.
| }), | ||
| listeners(({ actions, values }) => ({ | ||
| setErrorsToIgnoreRules: async ({ newRules }, breakpoint) => { | ||
| if (values.currentTeamErrorsToIgnoreRules === newRules.trim()) { |
There was a problem hiding this comment.
logic: comparing trimmed value against untrimmed value could cause unnecessary updates
| if (values.currentTeamErrorsToIgnoreRules === newRules.trim()) { | |
| if (values.currentTeamErrorsToIgnoreRules.trim() === newRules.trim()) { |
posthog/api/error_tracking.py
Outdated
| first_seen = serializers.DateTimeField(source="created_at") | ||
| # assignee: ErrorTrackingIssueAssignee | null |
There was a problem hiding this comment.
logic: first_seen is mapped to created_at, but this might not be accurate if issues can be merged. Consider if first_seen should be tracked separately.
|
Size Change: +69 B (+0.01%) Total Size: 1.18 MB ℹ️ View Unchanged
|
| class ErrorTrackingIssueSerializer(serializers.ModelSerializer): | ||
| # Might not be entirely accurate if an issue is merged but it's a good | ||
| # approximation to use in absence of doing a ClickHouse query | ||
| first_seen = serializers.DateTimeField(source="created_at") |
There was a problem hiding this comment.
We can use the lowest ErrorTrackingIssueFingerprint.created_at from the set of associated fingerprints here, and know it's exactly correct, right?
There was a problem hiding this comment.
Yes, good point. Might follow up with that if we want to add the first_seen field to that table anyways
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
In preparation for some wider changes we want to make about how events are displayed on issues. PR was getting too large so I decided to stop here
Changes