From 8303995c26fd48d6f4522d086a2130bc189d3111 Mon Sep 17 00:00:00 2001 From: Colm Cahalane Date: Thu, 6 Aug 2026 10:26:49 +0100 Subject: [PATCH 1/2] fix(scorecards): correct check output units and related properties Two defects in how check results render, both found by comparing the plugin against DX's own scorecard UI for the same entity. Custom output units were pluralized before display, but the unit is author-supplied and already written in its intended form. `pluralize()` appends "s" to anything outside its special-case list, so units came out doubled: "4 trace metricss", "89 charss". Render the unit verbatim, as DX does. `pluralize()` still serves the duration_* output types, whose units are hardcoded singulars. The related-property section never rendered for any check. The API returns `related_properties` as an array; the type and the drawer both expected a singular `related_property` string, which no response contains. Correct the field and render every property in the array. Also fixes `CustomOutputOptions.unit`, typed as the string literal "string" rather than `string`. --- src/api.ts | 4 +- src/components/CheckResultBadge.test.tsx | 77 +++++++++++++++++++++ src/components/CheckResultBadge.tsx | 6 +- src/components/CheckResultDrawer.test.tsx | 83 +++++++++++++++++++++++ src/components/CheckResultDrawer.tsx | 40 +++++++---- 5 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 src/components/CheckResultBadge.test.tsx create mode 100644 src/components/CheckResultDrawer.test.tsx diff --git a/src/api.ts b/src/api.ts index 3cd3976..3c16ca7 100644 --- a/src/api.ts +++ b/src/api.ts @@ -81,7 +81,7 @@ type CheckCommon = { published: boolean; output: Output | null; message: string | null; - related_property: string | null; + related_properties: string[] | null; passed: boolean; status: "PASS" | "FAIL" | "WARN"; executed_at: string | null; @@ -105,7 +105,7 @@ export type OutputType = | "custom"; export type CustomOutputOptions = { - unit: "string"; + unit: string; decimals: "auto" | number; }; diff --git a/src/components/CheckResultBadge.test.tsx b/src/components/CheckResultBadge.test.tsx new file mode 100644 index 0000000..a3ad458 --- /dev/null +++ b/src/components/CheckResultBadge.test.tsx @@ -0,0 +1,77 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import { CheckResultBadge, CheckResultBadgeProps } from "./CheckResultBadge"; + +function renderBadge(props: Partial = {}) { + return render( + + ); +} + +describe("CheckResultBadge", () => { + it("renders a custom unit verbatim rather than pluralizing it", () => { + renderBadge({ + outputValue: 4, + outputType: "custom", + outputCustomOptions: { unit: "trace metrics", decimals: 0 }, + }); + + expect(screen.getByText("4 trace metrics")).toBeInTheDocument(); + }); + + it("renders a custom unit verbatim when decimals are automatic", () => { + renderBadge({ + outputValue: 89, + outputType: "custom", + outputCustomOptions: { unit: "chars", decimals: "auto" }, + }); + + expect(screen.getByText("89 chars")).toBeInTheDocument(); + }); + + it("formats a custom value to the requested number of decimals", () => { + renderBadge({ + outputValue: 0, + outputType: "custom", + outputCustomOptions: { unit: "% time > 90% util", decimals: 2 }, + }); + + expect(screen.getByText("0.00 % time > 90% util")).toBeInTheDocument(); + }); + + it("pluralizes built-in duration units", () => { + renderBadge({ outputValue: 3, outputType: "duration_days" }); + + expect(screen.getByText("3 days")).toBeInTheDocument(); + }); + + it("keeps built-in duration units singular for a count of one", () => { + renderBadge({ outputValue: 1, outputType: "duration_days" }); + + expect(screen.getByText("1 day")).toBeInTheDocument(); + }); + + it("falls back to the status text when output is disabled", () => { + renderBadge({ + status: "FAIL", + outputEnabled: false, + outputValue: 4, + outputType: "number", + }); + + expect(screen.getByText("Not passed")).toBeInTheDocument(); + }); + + it("shows a no-data placeholder when output is enabled but empty", () => { + renderBadge({ outputType: "number" }); + + expect(screen.getByText("(No data)")).toBeInTheDocument(); + }); +}); diff --git a/src/components/CheckResultBadge.tsx b/src/components/CheckResultBadge.tsx index b94951c..62fb2db 100644 --- a/src/components/CheckResultBadge.tsx +++ b/src/components/CheckResultBadge.tsx @@ -230,13 +230,15 @@ function formatCustomOutputValue( outputValue: number, outputCustomOptions: CustomOutputOptions ): string { + // The unit is author-supplied and already in its intended form, so it is + // rendered verbatim rather than pluralized. if (outputCustomOptions.decimals === "auto") { - return `${outputValue} ${pluralize(outputCustomOptions.unit, outputValue)}`; + return `${outputValue} ${outputCustomOptions.unit}`; } const valueWithDecimals = outputValue.toFixed(outputCustomOptions.decimals); - return `${valueWithDecimals} ${pluralize(outputCustomOptions.unit, outputValue)}`; + return `${valueWithDecimals} ${outputCustomOptions.unit}`; } function pluralize(text: string, count: number | null = null) { diff --git a/src/components/CheckResultDrawer.test.tsx b/src/components/CheckResultDrawer.test.tsx new file mode 100644 index 0000000..b87f237 --- /dev/null +++ b/src/components/CheckResultDrawer.test.tsx @@ -0,0 +1,83 @@ +import React from "react"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; + +import { CheckResultDrawer } from "./CheckResultDrawer"; +import { LevelBasedScorecardCheck } from "../api"; + +const check: LevelBasedScorecardCheck = { + id: "rcw3pkmrxp8j", + name: "Internal dependencies", + description: "Set the internal-dependency-documentation DX property.", + published: true, + output: { type: "string", value: "Needs doc link" }, + message: null, + related_properties: null, + passed: false, + status: "FAIL", + executed_at: null, + level: { id: "92ktdhy45tls", name: "Required" }, +}; + +function renderDrawer({ + onEditRelatedProperty = () => {}, + ...overrides +}: Partial & { + onEditRelatedProperty?: () => void; +} = {}) { + return render( + {}} + onEditRelatedProperty={onEditRelatedProperty} + /> + ); +} + +describe("CheckResultDrawer", () => { + it("lists a single related property", () => { + renderDrawer({ + related_properties: ["internal-dependency-documentation"], + }); + + expect(screen.getByText("Related property:")).toBeInTheDocument(); + expect( + screen.getByText("internal-dependency-documentation") + ).toBeInTheDocument(); + }); + + it("lists every related property when a check has several", () => { + renderDrawer({ + related_properties: ["scaling-thresholds", "task-count"], + }); + + expect(screen.getByText("Related properties:")).toBeInTheDocument(); + expect(screen.getByText("scaling-thresholds")).toBeInTheDocument(); + expect(screen.getByText("task-count")).toBeInTheDocument(); + }); + + it("offers an edit affordance for the related properties", async () => { + const onEditRelatedProperty = jest.fn(); + renderDrawer({ + related_properties: ["internal-dependency-documentation"], + onEditRelatedProperty, + }); + + await userEvent.click(screen.getByRole("button", { name: "Edit in DX" })); + + expect(onEditRelatedProperty).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["null", null], + ["empty", []], + ])("omits the related property section when %s", (_label, related) => { + renderDrawer({ related_properties: related as string[] | null }); + + expect(screen.queryByText(/^Related propert/)).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Edit in DX" }) + ).not.toBeInTheDocument(); + }); +}); diff --git a/src/components/CheckResultDrawer.tsx b/src/components/CheckResultDrawer.tsx index 4d0e8cf..e569e7e 100644 --- a/src/components/CheckResultDrawer.tsx +++ b/src/components/CheckResultDrawer.tsx @@ -99,7 +99,7 @@ export function CheckResultDrawer({ )} - {check.related_property && ( + {!!check.related_properties?.length && ( - - Related property:{" "} - - {check.related_property} - - + + + {check.related_properties.length === 1 + ? "Related property:" + : "Related properties:"} + + {check.related_properties.map((relatedProperty) => ( + + {relatedProperty} + + ))} +