Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev 349 forreal #125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 105 additions & 1 deletion src/app/pages/server-compare/server-compare.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ <h2 class="text-xl font-semibold">{{category.name}}</h2>
</ng-container>
</tr>
</ng-container>

<!--
<ng-container *ngFor="let category of benchmarkCategories">
<ng-container *ngIf="category.data.length">
<tr class="section_header" id="benchmark_line">
Expand Down Expand Up @@ -506,6 +506,110 @@ <h3 class="text-xl font-semibold flex gap-2 items-center">{{category.name}}
</ng-container>
</ng-container>
</ng-container>
-->
<ng-container *ngFor="let chartItem of multiBarCharts">
<ng-container *ngIf="chartItem.chart.chartData">
<tr class="section_header" id="benchmark_line">
<td [colSpan]="2" >
<h3 class="text-xl font-semibold flex gap-2 items-center">FOOBAR {{chartItem.chart.name}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove Placeholder Text "FOOBAR" from Header

The header at line 514 contains a hardcoded string "FOOBAR" which appears to be a placeholder or leftover from debugging. It should be removed to ensure the chart displays the correct name.

Apply this diff to fix the issue:

-        <h3 class="text-xl font-semibold flex gap-2 items-center">FOOBAR {{chartItem.chart.name}}
+        <h3 class="text-xl font-semibold flex gap-2 items-center">{{chartItem.chart.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
<h3 class="text-xl font-semibold flex gap-2 items-center">FOOBAR {{chartItem.chart.name}}
<h3 class="text-xl font-semibold flex gap-2 items-center">{{chartItem.chart.name}}

<lucide-icon
*ngIf="chartItem.chart.options[chartItem.chart.selectedOption].icon"
name="{{chartItem.chart.options[chartItem.chart.selectedOption].icon}}"
class="h-4 w-4 text-gray-200"
(mouseenter)="showTooltip($event, chartItem.chart.options[chartItem.chart.selectedOption].tooltip)"
(mouseleave)="hideTooltip()">
</lucide-icon>
<lucide-icon
name="info"
class="h-4 w-4 text-gray-200"
(mouseenter)="showTooltipChart($event, chartItem.chart.options[chartItem.chart.selectedOption].benchmark_id)"
(mouseleave)="hideTooltip()">
</lucide-icon>
</h3>
</td>
<td [colSpan]="servers.length -1">

</td>
</tr>
<tr>
<td [colSpan]="servers.length + 1" >
<div>
<div class="block_full_inline" *ngIf="chartItem.chart.chartData">
<div class="flex justify-end items-center">
<button
id="bw_mem_button"
class="dropdown_button"
Comment on lines +540 to +541
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix Duplicate IDs and Incorrect Variable References in Button Element

Within the ngFor loop over chartItem, the button element uses a hardcoded id="bw_mem_button" at line 540. This can lead to duplicate IDs in the rendered HTML, which is invalid and may cause issues with JavaScript event handling or styling.

Additionally, the button displays {{selectedBWMemOperation.name}} at line 544, which may not be defined within the scope of the loop. It should refer to the corresponding property of chartItem to display the correct operation name for each chart.

To resolve these issues, modify the id to be unique or remove it if it's not needed, and update the variable reference to use chartItem's context.

Apply this diff:

-                              <button
-                                id="bw_mem_button"
+                              <button
+                                [id]="'bw_mem_button_' + chartItem.chart.name"

And update the operation name display:

-                                    {{selectedBWMemOperation.name}}
+                                    {{chartItem.chart.options[chartItem.chart.selectedOption].name}}

Also applies to: 543-545

type="button">
<div class="px-4 py-3">
{{selectedBWMemOperation.name}}
</div>
<div class="px-4 py-3 w-12 h-full bg-primary justify-center items-center flex rounded-r-lg">
<lucide-icon name="chevron-down" class="h-6 w-6 text-emerald-400"></lucide-icon>
Comment on lines +543 to +547
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Correct Data Binding for Chart Options

The chart options within the button should reference the correct properties from chartItem to display dynamically based on the current chart item.

Update the code to use the appropriate chartItem properties:

-                                  <div class="px-4 py-3">
-                                    {{selectedBWMemOperation.name}}
+                                  <div class="px-4 py-3">
+                                    {{chartItem.chart.options[chartItem.chart.selectedOption].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
<div class="px-4 py-3">
{{selectedBWMemOperation.name}}
</div>
<div class="px-4 py-3 w-12 h-full bg-primary justify-center items-center flex rounded-r-lg">
<lucide-icon name="chevron-down" class="h-6 w-6 text-emerald-400"></lucide-icon>
<div class="px-4 py-3">
{{chartItem.chart.options[chartItem.chart.selectedOption].name}}
</div>
<div class="px-4 py-3 w-12 h-full bg-primary justify-center items-center flex rounded-r-lg">
<lucide-icon name="chevron-down" class="h-6 w-6 text-emerald-400"></lucide-icon>

</div>
</button>
</div>
<div *ngIf="isBrowser()">
<div chartType="bar">
<canvas
style="height: 350px; max-height: 350px; max-width:100%"
baseChart
[data]="chartItem.chart.chartData"
[options]="chartItem.chart.chartOptions"
[type]="chartItem.chart.chartType"
>
</canvas>
</div>
</div>
</div>
</div>
<div class="mt-4 mb-8 flex justify-center"
[ngClass]="{
'mb-8': !chartItem.show_more
}">
<button class="btn-primary py-2 px-3" (click)="toggleBenchmarkCategory(chartItem)">
{{chartItem.show_more ? 'Hide' : 'Show'}} Details
</button>
</div>
</td>
</tr>
<ng-container *ngIf="chartItem.show_more">
<ng-container *ngFor="let item of chartItem.data">
<tr *ngIf="chartItem.data.length > 1">
<td [colSpan]="servers.length + 1" >
<div class="ml-4 flex justify-between">
<div class="flex gap-2 items-center">
{{item.name}}
<lucide-icon
*ngIf="item.description"
name="info"
class="h-4 w-4 text-gray-200"
(mouseenter)="showTooltip($event, item.description)"
(mouseleave)="hideTooltip()">
</lucide-icon>
</div>
<lucide-icon
*ngIf="item.description"
[name]="benchmarkIcon(item)"
class="h-6 w-6 text-emerald-400 cursor-pointer"
(click)="toggleBenchmark(item)"
>
</lucide-icon>
</div>
</td>
</tr>
<ng-container *ngIf="!item.collapsed" >
<tr *ngFor="let config of item.configs">
<td>
<div class="ml-6" [innerHTML]="serializeConfig(config.config)" ></div></td>
<td *ngFor="let value of config.values" [style]="isBestStyle(value, config.values, item)">
{{numberWithCommas(value)}}
</td>
</tr>
</ng-container>
</ng-container>
</ng-container>
</ng-container>
</ng-container>

<ng-container *ngIf="getBenchmarksForCategory(null)?.length">
<tr class="section_header">
Expand Down
109 changes: 96 additions & 13 deletions src/app/pages/server-compare/server-compare.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { ServerCompareService } from '../../services/server-compare.service';
import { DropdownManagerService } from '../../services/dropdown-manager.service';
import { AnalyticsService } from '../../services/analytics.service';
import { CurrencyOption, availableCurrencies } from '../../tools/shared_data';
import { ChartFromBenchmarkTemplate, ChartFromBenchmarkTemplateOptions, redisChartTemplate, redisChartTemplateCallbacks, staticWebChartCompareTemplate, staticWebChartTemplate, staticWebChartTemplateCallbacks } from '../server-details/chartFromBenchmarks';

Chart.register(annotationPlugin);

Expand Down Expand Up @@ -165,6 +166,23 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {

selectedRedisOption: any = this.redisOptions[0];

multiBarCharts: any[] = [
{
chart: JSON.parse(JSON.stringify(redisChartTemplate)),
callbacks: redisChartTemplateCallbacks,
dropdown: undefined,
data: [],
show_more: false,
},
{
chart: JSON.parse(JSON.stringify(staticWebChartCompareTemplate)),
callbacks: staticWebChartTemplateCallbacks,
dropdown: undefined,
data: [],
show_more: false,
}
];

Comment on lines +169 to +185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Specify types and avoid inefficient object cloning in multiBarCharts

The multiBarCharts array is declared with the type any[]. Defining a specific interface or type for the chart items would improve type safety and maintainability.

Additionally, using JSON.parse(JSON.stringify(...)) for deep cloning is inefficient and can lead to issues with functions or complex objects. Consider using the spread operator or a utility function like _.cloneDeep from lodash for deep cloning.

Example:

// Define an interface for chart items
interface MultiBarChartItem {
  chart: ChartFromBenchmarkTemplate;
  callbacks: any;
  dropdown?: any;
  data: any[];
  show_more: boolean;
}

// Declare the array with the specific type
multiBarCharts: MultiBarChartItem[] = [
  {
    chart: { ...redisChartTemplate },
    callbacks: redisChartTemplateCallbacks,
    dropdown: undefined,
    data: [],
    show_more: false,
  },
  {
    chart: { ...staticWebChartCompareTemplate },
    callbacks: staticWebChartTemplateCallbacks,
    dropdown: undefined,
    data: [],
    show_more: false,
  }
];

compressDropdown: any;
availableCompressMethods: any[] = [];
selectedCompressMethod: any;
Expand Down Expand Up @@ -401,20 +419,38 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
category.data = this.benchmarkMeta.filter((b: any) => category.benchmarks.includes(b.benchmark_id));
});

this.multiBarCharts.forEach((chartTemplate) => {
const benchmarks = chartTemplate.chart.options.map((o: any) => o.benchmark_id);
chartTemplate.data = this.benchmarkMeta.filter((b: any) => benchmarks.includes(b.benchmark_id));
});

if(isPlatformBrowser(this.platformId)) {

this.initializeBenchmarkCharts();

this.getCompressChartOptions();

this.generateChartsData();
this.generateBWMemChart();
this.generateSSLChart();
this.generateCompressChart();
this.generateStatiWebChart(this.selectedStaticWebOption, this.selectedConnections);
this.generateStatiWebChart(this.selectedRedisOption, this.selectedOperation);
//this.generateMultiBarChart(this.selectedStaticWebOption, this.selectedConnections);
//this.generateMultiBarChart(this.selectedRedisOption, this.selectedOperation);

this.multiBarCharts.forEach((chart) => {
this.generateMultiBarChart(chart.chart);
});
Comment on lines +440 to +442
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify the argument passed to generateMultiBarChart

In the loop, this.generateMultiBarChart(chart.chart); is called. Ensure that chart.chart is the correct argument and matches the expected type ChartFromBenchmarkTemplate. If chart itself is the ChartFromBenchmarkTemplate, you may need to pass chart instead.

Apply this fix:

-          this.generateMultiBarChart(chart.chart);
+          this.generateMultiBarChart(chart);

Committable suggestion was skipped due to low confidence.


this.dropdownManager.initDropdown('currency_button', 'currency_options').then((dropdown) => {
this.dropdownCurrency = dropdown;
});

this.multiBarCharts.forEach((chart) => {
this.dropdownManager.initDropdown(chart.chart.id + '_button', chart.chart.id + '_options').then((dropdown) => {
chart.dropdown = dropdown;
});
});

}
}).catch((err) => {
this.analytics.SentryException(err, {tags: { location: this.constructor.name, function: 'compareInit' }});
Expand Down Expand Up @@ -915,14 +951,23 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
}
}

generateStatiWebChart(chartConf: any, chartConf2: any) {
generateMultiBarChart(chartTemplate: ChartFromBenchmarkTemplate) {

const benchmark_id = chartConf.benchmark;
const labelsField = chartConf.scaleField;
const scaleField = chartConf.labelsField;
const option: ChartFromBenchmarkTemplateOptions = chartTemplate.options[chartTemplate.selectedOption];
const benchmark_id = option.benchmark_id;
const labelsField = option.labelsField;
const scaleField = option.scaleField;

const labelValue = chartConf2.value;
const selectedName = chartConf2.name;
if(!chartTemplate.secondaryOptions || chartTemplate.selectedSecondaryOption === undefined) {
return;
}

const optionsSecondary = chartTemplate.secondaryOptions[chartTemplate.selectedSecondaryOption];

console.log(optionsSecondary, labelsField);

const labelValue = optionsSecondary.value;
const selectedName = optionsSecondary.name;

const dataSet = this.benchmarkMeta?.find((x: any) => x.benchmark_id === benchmark_id);

Expand Down Expand Up @@ -966,7 +1011,7 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
chartData.datasets[i].data.push({
data:item.score,
label: size,
unit: chartConf.YLabel,
unit: option.YLabel,
note: item.note
});
} else {
Expand All @@ -975,6 +1020,19 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
});
});

console.log(chartData);

if(chartData) {
chartTemplate.chartData = { labels: chartData.labels, datasets: chartData.datasets };
chartTemplate.chartOptions.scales.y.title.text = option.YLabel;
chartTemplate.chartOptions.scales.x.title.text = option.XLabel;
chartTemplate.chartOptions.plugins.title.text = option.title;
} else {
chartTemplate.chartData = undefined;
}


/*
if(benchmark_id.includes('static_web')) {
this.barChartDataStaticWeb = { labels: chartData.labels, datasets: chartData.datasets };

Expand Down Expand Up @@ -1007,6 +1065,7 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
});
}
}
*/
}
}

Expand Down Expand Up @@ -1140,6 +1199,30 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {
}
}

initializeBenchmarkCharts() {

this.multiBarCharts.forEach((chartItem: any) => {
chartItem.chart.chartOptions.plugins.tooltip.callbacks = chartItem.callbacks;
this.initializeMultiBarChart(chartItem.chart);
});
}

initializeMultiBarChart(chartTemplate: ChartFromBenchmarkTemplate) {
chartTemplate.options.forEach((option: ChartFromBenchmarkTemplateOptions) => {
const benchmark = this.benchmarkMeta.find((b: any) => b.benchmark_id === option.benchmark_id);
if(benchmark) {
option.name = benchmark.name;
option.unit = benchmark.unit;
option.higher_is_better = benchmark.higher_is_better;
option.icon = benchmark.higher_is_better ? 'circle-arrow-up' : 'circle-arrow-down';
option.tooltip = benchmark.higher_is_better ? 'Higher is better' : 'Lower is better';
option.title = ' '
option.XLabel = benchmark.config_fields ? ((benchmark.config_fields as any)[option.scaleField] as string) : '';
option.YLabel = benchmark.unit;
}
});
}

isBrowser() {
return isPlatformBrowser(this.platformId);
}
Expand Down Expand Up @@ -1195,25 +1278,25 @@ export class ServerCompareComponent implements OnInit, AfterViewInit {

selectStaticWebOption(item: any) {
this.selectedStaticWebOption = item;
this.generateStatiWebChart(this.selectedStaticWebOption, this.selectedConnections);
//this.generateMultiBarChart(this.selectedStaticWebOption, this.selectedConnections);
this.dropdownStaticWeb?.hide();
}

selectStaticWebConnOption(item: any) {
this.selectedConnections = item;
this.generateStatiWebChart(this.selectedStaticWebOption, this.selectedConnections);
//this.generateMultiBarChart(this.selectedStaticWebOption, this.selectedConnections);
this.dropdownConnections?.hide();
}

selectRedisOption(item: any) {
this.selectedRedisOption = item;
this.generateStatiWebChart(this.selectedRedisOption, this.selectedOperation);
//this.generateMultiBarChart(this.selectedRedisOption, this.selectedOperation);
this.dropdownRedis?.hide();
}

selectRedisOpOption(item: any) {
this.selectedOperation = item;
this.generateStatiWebChart(this.selectedRedisOption, this.selectedOperation);
//this.generateMultiBarChart(this.selectedRedisOption, this.selectedOperation);
this.dropdownOperation?.hide();
}

Expand Down
Loading
Loading