209 scatterplot on seats page that displays devs providing estimates on a grid #237
Conversation
…pilotSeatComponent
…devs-providing-estimates-on-a-grid
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new scatter chart visualization for seat activity data on the seats page. The changes refactor the CopilotSeatComponent to support dual chart display (Gantt and scatter), move inline styles from HTML to SCSS for better maintainability, and add a new transformSeatActivityToScatter method to the HighchartsService.
- Added a new scatter chart to display developer activity and time saved metrics
- Refactored the component's data loading logic to be more streamlined
- Migrated inline styles from HTML to SCSS file
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| frontend/src/app/services/highcharts.service.ts | Added transformSeatActivityToScatter method to generate scatter chart configuration; minor whitespace cleanup |
| frontend/src/app/main/copilot/copilot-seats/copilot-seat/copilot-seat.component.ts | Added chart2Options property for second chart; refactored data loading logic to simplify nested subscriptions |
| frontend/src/app/main/copilot/copilot-seats/copilot-seat/copilot-seat.component.scss | Added styles previously defined inline in HTML template |
| frontend/src/app/main/copilot/copilot-seats/copilot-seat/copilot-seat.component.html | Added new chart card with scatter chart; removed inline <style> block in favor of SCSS |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chart2Options: Highcharts.Options = { | ||
| title: { | ||
| text: "Seat Activity by Editor" | ||
| }, | ||
| xAxis: { | ||
| type: "datetime" | ||
| }, | ||
| legend: { | ||
| enabled: false | ||
| }, | ||
| series: [ | ||
| { | ||
| name: "Seat Activity", | ||
| type: "gantt", | ||
| data: [] | ||
| } | ||
| ], | ||
| plotOptions: { | ||
| gantt: { | ||
| borderWidth: 0, | ||
| borderColor: undefined, | ||
| dataLabels: { | ||
| enabled: true | ||
| } | ||
| } | ||
| }, | ||
| tooltip: {}, | ||
| yAxis: { | ||
| categories: [ | ||
| "vscode", | ||
| "copilot-summarization-pr" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
The chart2Options initialization duplicates chartOptions with identical configuration (gantt type, same title, same structure). Since chart2Options is meant to display a scatter chart, this initialization should be updated to reflect scatter chart configuration or removed if the configuration will be completely replaced by the transformSeatActivityToScatter result.
| const parts = [ | ||
| `<b>Editor:</b> ${this.raw?.last_activity_editor || 'unknown'}<br/>`, | ||
| `<b>Activity:</b> ${new Date(this.x).toLocaleString()}<br/>`, | ||
| `<b>Time Saved:</b> ${this.y} minutes` |
There was a problem hiding this comment.
The tooltip displays "Time Saved: {value} minutes" but the y-axis value is generated using Math.random() * 100 without any unit context, and the y-axis label format uses 'kg'. These inconsistencies will confuse users. Ensure the tooltip, axis labels, and data values all use consistent units.
| } | ||
|
|
||
| transformSeatActivityToScatter(seatActivity: Seat[]): Highcharts.Options { | ||
| console.log(seatActivity); |
There was a problem hiding this comment.
The console.log statement should be removed from production code. This appears to be a debugging statement that was left in.
| console.log(seatActivity); |
| labels: { | ||
| format: '{value} m' | ||
| }, | ||
| startOnTick: true, | ||
| endOnTick: true, | ||
| showLastLabel: true | ||
| }, | ||
| yAxis: { | ||
| title: { | ||
| text: 'Time Saved' | ||
| }, | ||
| labels: { | ||
| format: '{value} kg' | ||
| } |
There was a problem hiding this comment.
The axis label format is incorrect. The x-axis represents datetime (activity timestamps) but is formatted as '{value} m' (minutes), and the y-axis represents time saved but is formatted as '{value} kg' (kilograms). The x-axis should use datetime formatting, and the y-axis should use 'minutes' or another time unit instead of 'kg'.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| type: 'scatter' as const, | ||
| data: seatActivity.map(seat => ({ | ||
| x: new Date(seat.last_activity_at || seat.created_at).getTime(), | ||
| y: Math.random() * 100, // Placeholder for time saved, adjust as needed |
There was a problem hiding this comment.
Using Math.random() for the y-axis value is problematic for a production feature. This creates non-deterministic data that will change on every render and doesn't represent actual time saved. If the actual time saved metric is not yet available, consider using a real metric from the seat data or providing a clear TODO comment indicating what metric should be used here.
| y: Math.random() * 100, // Placeholder for time saved, adjust as needed | |
| // TODO: Replace with actual time saved metric from seat data when available | |
| y: seat.time_saved ?? 0, // Currently defaults to 0 if not available |
| xAxis: { | ||
| title: { | ||
| text: 'Activity' | ||
| }, | ||
| labels: { | ||
| format: '{value} m' | ||
| }, | ||
| startOnTick: true, | ||
| endOnTick: true, | ||
| showLastLabel: true | ||
| }, |
There was a problem hiding this comment.
The xAxis configuration is missing the required type: 'datetime' property. Since the x-axis represents timestamps (via new Date(...).getTime()), it should be explicitly configured as a datetime axis for proper formatting and tick placement.
| })), | ||
| tooltip: { | ||
| headerFormat: '<b>{point.name}</b><br/>', | ||
| pointFormatter: function (this: CustomHighchartsPoint) { |
There was a problem hiding this comment.
[nitpick] Trailing spaces on this line should be removed.
| pointFormatter: function (this: CustomHighchartsPoint) { | |
| pointFormatter: function (this: CustomHighchartsPoint) { |
| if (!this.id) return; | ||
|
|
||
| // Show loading indicator - SET TO TRUE at the start of data loading | ||
| this.loading = true; |
There was a problem hiding this comment.
The removal of this.cdr.detectChanges() after setting this.loading = true may prevent the loading indicator from displaying immediately. Since the component uses ChangeDetectionStrategy.OnPush, manual change detection is needed to update the view. Consider keeping this call to ensure the loading state is reflected in the UI before the async operations begin.
| this.loading = true; | |
| this.loading = true; | |
| this.cdr.detectChanges(); |
|
@austenstone I've opened a new pull request, #262, to work on those changes. Once the pull request is ready, I'll request review from you. |
This pull request introduces a new scatter chart to display seat activity data, refactors the
CopilotSeatComponentto simplify its logic, and moves inline styles to the SCSS file for better maintainability. Below is a summary of the most important changes: