From 3a197d015d947e742d50982d3a17c2bf090621b7 Mon Sep 17 00:00:00 2001 From: Philipp Schneider Date: Tue, 3 Mar 2026 10:47:06 +0100 Subject: [PATCH 1/4] fix of cursor pointer --- src/core/chart-api/chart-extra-pointer.tsx | 15 ++++++--------- src/core/styles.scss | 8 ++++++++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/core/chart-api/chart-extra-pointer.tsx b/src/core/chart-api/chart-extra-pointer.tsx index 305f3e12..1e297e18 100644 --- a/src/core/chart-api/chart-extra-pointer.tsx +++ b/src/core/chart-api/chart-extra-pointer.tsx @@ -8,6 +8,8 @@ import { DebouncedCall } from "../../internal/utils/utils"; import { isPointVisible } from "../utils"; import { ChartExtraContext } from "./chart-extra-context"; +import styles from "../styles.css.js"; + const HOVER_LOST_DELAY = 25; export interface ChartExtraPointerHandlers { @@ -186,15 +188,10 @@ export class ChartExtraPointer { private applyCursorStyle = () => { const container = this.context.chart().container; - const setCursor = (value: "pointer" | "default") => { - if (container && container.style.cursor !== value) { - container.style.cursor = value; - } - }; - if (this.hoveredPoint || this.hoveredGroup) { - setCursor("pointer"); - } else { - setCursor("default"); + if (container) { + const isHovered = !!(this.hoveredPoint || this.hoveredGroup); + container.classList.toggle(styles["cursor-pointer"], isHovered); + container.classList.toggle(styles["cursor-default"], !isHovered); } }; } diff --git a/src/core/styles.scss b/src/core/styles.scss index d9c5f23d..075d7171 100644 --- a/src/core/styles.scss +++ b/src/core/styles.scss @@ -131,3 +131,11 @@ $side-legend-max-inline-size: 30%; // We hide the native focus outline to render a custom one around the chart plot instead. outline: none; } + +.cursor-pointer { + cursor: pointer; +} + +.cursor-default { + cursor: default; +} From ffe7e02848677483bd471fd3d966b38241b68445 Mon Sep 17 00:00:00 2001 From: Philipp Schneider Date: Wed, 4 Mar 2026 10:01:53 +0100 Subject: [PATCH 2/4] fix: resolve CSP violation caused by inline style attributes on SVG elements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When hovering over the chart, Highcharts' SVG renderer calls element.setAttribute('style', ...) for SVG elements that have a 'style' string attribute. This violates Content Security Policy when 'style-src' is set to 'self', because setAttribute('style', ...) is treated as an inline style by the browser's CSP enforcement. The fix replaces 'style: "pointer-events: none"' string attributes with direct SVG presentation attributes ('pointer-events': 'none'), which Highcharts passes to setAttribute('pointer-events', 'none') instead — a valid SVG attribute that does not trigger CSP violations. Files changed: - src/core/chart-api/chart-extra-tooltip.tsx: tooltip track elements - src/internal/chart-styles.ts: cursor line element - src/internal/components/series-marker/render-marker.tsx: highlight markers Also reverts the previous cursor CSS class approach, as container.style.cursor (CSSOM manipulation) is not blocked by CSP. Resolves: AWSUI-61770 --- docs/in-operator-review-AWSUI-59006.md | 279 ++++++++++++++++ docs/tech-proposal-in-operator-audit.md | 316 ++++++++++++++++++ src/core/chart-api/chart-extra-pointer.tsx | 7 +- src/core/chart-api/chart-extra-tooltip.tsx | 2 +- src/core/styles.scss | 8 - src/internal/chart-styles.ts | 2 +- .../series-marker/render-marker.tsx | 4 +- 7 files changed, 601 insertions(+), 17 deletions(-) create mode 100644 docs/in-operator-review-AWSUI-59006.md create mode 100644 docs/tech-proposal-in-operator-audit.md diff --git a/docs/in-operator-review-AWSUI-59006.md b/docs/in-operator-review-AWSUI-59006.md new file mode 100644 index 00000000..e3df79e3 --- /dev/null +++ b/docs/in-operator-review-AWSUI-59006.md @@ -0,0 +1,279 @@ +# `in` Operator Audit Findings — AWSUI-59006 + +## Repository: `cloudscape-design/chart-components` + +**Audit date:** 2025-02-19 +**GitHub base URL:** https://github.com/cloudscape-design/chart-components/tree/main/src + +Related: [Tech proposal](./tech-proposal-in-operator-audit.md) + +--- + +## Summary + +| Metric | Count | +|--------|-------| +| Total `'prop' in obj` usages (source) | 12 | +| Category 1 — Union type discrimination | **0** | +| Category 2 — Runtime/feature detection | **12** | +| Test file usages | 1 | +| Type guards to create | **0** | +| Actionable items | **0** | + +**All 12 usages are Category 2 and MUST stay as-is.** No code changes required. + +--- + +## Detailed findings + +### Usage #1: `"symbol" in series` + +**File:** [`src/core/utils.ts:92`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L92) +**Function:** `getSeriesMarkerType()` +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `Highcharts.Series.symbol` is not in the TS type definition but exists at runtime for scatter series markers. + +```ts +// src/core/utils.ts, lines 88–96 +export function getSeriesMarkerType(series?: Highcharts.Series): ChartSeriesMarkerType { + if (!series) { + return "large-square"; + } + const seriesSymbol = "symbol" in series && typeof series.symbol === "string" ? series.symbol : "circle"; + // In Highcharts, dashStyle supports different types of dashes + if ("dashStyle" in series.options && series.options.dashStyle && series.options.dashStyle !== "Solid") { + return "dashed"; + } +``` + +--- + +### Usage #2: `"dashStyle" in series.options` + +**File:** [`src/core/utils.ts:94`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L94) +**Function:** `getSeriesMarkerType()` +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `dashStyle` exists on some Highcharts series types but not in the base `SeriesOptions` type. + +```ts +// src/core/utils.ts, lines 94–96 + if ("dashStyle" in series.options && series.options.dashStyle && series.options.dashStyle !== "Solid") { + return "dashed"; + } +``` + +--- + +### Usage #3: `"type" in series` + +**File:** [`src/core/utils.ts:193`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L193) +**Function:** `getVisibleLegendItems()` +**Category:** 2 — Highcharts complex external union +**Why it must stay:** `Highcharts.SeriesOptionsType` is a union of 50+ series option types; `type` may or may not be explicitly present. + +```ts +// src/core/utils.ts, lines 191–196 + options.series?.forEach((series) => { + if (!("type" in series)) { + return; + } + + // The pie series is not shown in the legend. Instead, we show pie segments (points). +``` + +--- + +### Usage #4: `"xAxis" in item || "yAxis" in item` + +**File:** [`src/core/utils.ts:208`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L208) +**Function:** `isSecondaryLegendItem()` +**Category:** 2 — Highcharts complex external union +**Why it must stay:** Duck-typing on `Highcharts.SeriesOptionsType | Highcharts.PointOptionsType` — external union not controlled by this codebase. + +```ts +// src/core/utils.ts, lines 206–211 + // Only items that have an associated axis can be considered secondary + if (item && typeof item === "object" && ("xAxis" in item || "yAxis" in item)) { + const axisRef = isInverted ? (item.xAxis ?? 0) : (item.yAxis ?? 0); + // An axis reference can be an index into the axes array or an explicitly passed id + const valueAxis = typeof axisRef === "number" ? valueAxes[axisRef] : valueAxes.find((a) => a.id === axisRef); + return valueAxis?.opposite ?? false; +``` + +--- + +### Usage #5: `"whiskers" in point` + +**File:** [`src/core/utils.ts:275`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L275) +**Function:** `getErrorBarPointRect()` +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `Highcharts.Point.whiskers` SVG element exists only on errorbar points at runtime, not in TS types. + +```ts +// src/core/utils.ts, lines 272–278 +function getErrorBarPointRect(point: Highcharts.Point): Rect { + const chart = point.series.chart; + if ("whiskers" in point) { + return getChartRect((point.whiskers as Highcharts.SVGElement).getBBox(), chart, true); + } + return getPointRectFromCoordinates(point); +} +``` + +--- + +### Usage #6: `"plotLinesAndBands" in axis` + +**File:** [`src/core/chart-api/chart-extra-highlight.ts:187`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-highlight.ts#L187) +**Function:** `iteratePlotLines()` +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `Highcharts.Axis.plotLinesAndBands` array is explicitly not covered by TS types. The code has an inline comment: *"The Highcharts `Axis.plotLinesAndBands` API is not covered with TS."* + +```ts +// src/core/chart-api/chart-extra-highlight.ts, lines 185–193 +function iteratePlotLines(chart: Highcharts.Chart, cb: (lineId: string, line: Highcharts.PlotLineOrBand) => void) { + chart.axes?.forEach((axis) => { + // The Highcharts `Axis.plotLinesAndBands` API is not covered with TS. + if ("plotLinesAndBands" in axis && Array.isArray(axis.plotLinesAndBands)) { + axis.plotLinesAndBands.forEach((line: Highcharts.PlotLineOrBand) => { + if (line.options.id) { + cb(line.options.id, line); + } + }); +``` + +--- + +### Usage #7: `"tooltipPos" in point` + +**File:** [`src/core/chart-api/chart-extra-tooltip.tsx:208`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-tooltip.tsx#L208) +**Function:** `getPieChartTargetPlacement()` +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `Highcharts.Point.tooltipPos` tuple exists only on pie series points at runtime, not in TS types. The code includes a fallback path for when this property is absent. + +```ts +// src/core/chart-api/chart-extra-tooltip.tsx, lines 205–213 +function getPieChartTargetPlacement(point: Highcharts.Point): Rect { + // The pie series segments do not provide plotX, plotY to compute the tooltip placement. + // Instead, there is a `tooltipPos` tuple, which is not covered by TS. + // See: https://github.com/highcharts/highcharts/issues/23118. + if ("tooltipPos" in point && Array.isArray(point.tooltipPos)) { + return safeRect({ x: point.tooltipPos[0], y: point.tooltipPos[1], width: 0, height: 0 }); + } + // We use the alternative, middle, tooltip placement as a fallback + return getPieMiddlePlacement(point); +``` + +--- + +### Usage #8: `"data" in s.options` + +**File:** [`src/core/chart-api/chart-extra-nodata.tsx:55`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L55) +**Function:** `findAllSeriesWithData()` +**Category:** 2 — Highcharts complex external union +**Why it must stay:** Defensive check on `Highcharts.SeriesOptions` — `data` property may or may not be present depending on series type. + +```ts +// src/core/chart-api/chart-extra-nodata.tsx, lines 53–57 +function findAllSeriesWithData(chart: Highcharts.Chart) { + return getChartSeries(chart.series).filter((s) => { + const data = "data" in s.options && s.options.data && Array.isArray(s.options.data) ? s.options.data : []; + return data.some((i) => i !== null && (typeof i === "object" && "y" in i ? i.y !== null : true)); + }); +``` + +--- + +### Usage #9: `"y" in i` + +**File:** [`src/core/chart-api/chart-extra-nodata.tsx:56`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L56) +**Function:** `findAllSeriesWithData()` +**Category:** 2 — Highcharts complex external union +**Why it must stay:** Differentiating Highcharts data formats — data items can be `number | [number, number] | { y: number } | null`. + +```ts +// src/core/chart-api/chart-extra-nodata.tsx, line 56 + return data.some((i) => i !== null && (typeof i === "object" && "y" in i ? i.y !== null : true)); +``` + +--- + +### Usage #10: `"colorCounter" in chart` + +**File:** [`src/core/chart-api/index.tsx:290`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/index.tsx#L290) +**Function:** `ChartAPI.resetColorCounter()` (private method) +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** `Highcharts.Chart.colorCounter` is an internal property used for color assignment. Part of a workaround for [Highcharts bug #23077](https://github.com/highcharts/highcharts/issues/23077). + +```ts +// src/core/chart-api/index.tsx, lines 287–292 + private resetColorCounter() { + const chart = this.context.chart(); + if ("colorCounter" in chart && typeof chart.colorCounter === "number") { + chart.colorCounter = getChartSeries(chart.series).length; + } + } +``` + +--- + +### Usage #11: `"scrollTo" in element` + +**File:** [`src/internal/utils/dom.ts:18`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/utils/dom.ts#L18) +**Function:** `scrollIntoViewNearestContainer()` +**Category:** 2 — Browser API feature detection +**Why it must stay:** `scrollTo` is not available in JSDOM test environments. Classic feature detection pattern. + +```ts +// src/internal/utils/dom.ts, lines 16–20 + // Scroll methods (scrollTo, scrollIntoView, etc) are not supported in test environments like JSDOM. + if (!("scrollTo" in element)) { + return; + } +``` + +--- + +### Usage #12: `"symbol" in series` (duplicate) + +**File:** [`src/internal/components/series-marker/render-marker.tsx:82`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/components/series-marker/render-marker.tsx#L82) +**Function:** `getSeriesMarkerType()` (local to render-marker) +**Category:** 2 — Undocumented Highcharts property +**Why it must stay:** Same pattern as Usage #1 — undocumented `Highcharts.Series.symbol`, duplicate in a separate marker rendering module. + +```ts +// src/internal/components/series-marker/render-marker.tsx, lines 80–84 +function getSeriesMarkerType(series: Highcharts.Series): ChartSeriesMarkerType { + const seriesSymbol = "symbol" in series && typeof series.symbol === "string" ? series.symbol : "circle"; + if (series.type === "scatter") { + switch (seriesSymbol) { +``` + +--- + +### Test file usage: `"asymmetricMatch" in value` + +**File:** [`src/core/__tests__/common.tsx:72`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/__tests__/common.tsx#L72) +**Function:** `isJestMatcher()` (local helper in `objectContainingDeep`) +**Category:** 2 — Runtime Jest matcher detection +**Why it must stay:** `asymmetricMatch` is a Jest convention property not in any TS type. + +```ts +// src/core/__tests__/common.tsx, lines 70–74 +export function objectContainingDeep(root: object) { + function isJestMatcher(value: object): boolean { + return "asymmetricMatch" in value; + } + function transformLevel(obj: object) { +``` + +--- + +## Additional patterns found + +| Pattern | Count | Location | +|---------|-------|----------| +| `for...in` loops | 0 | — | +| `Object.keys()` | 1 | [`src/internal/base-component/get-data-attributes.ts`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/base-component/get-data-attributes.ts) — iterating `data-*` attributes from props | +| `Object.entries()` | 1 | [`src/core/chart-api/chart-extra-navigation.ts`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-navigation.ts) — iterating element attributes for `setAttribute` | +| `hasOwnProperty` | 0 | — | \ No newline at end of file diff --git a/docs/tech-proposal-in-operator-audit.md b/docs/tech-proposal-in-operator-audit.md new file mode 100644 index 00000000..d44eafb1 --- /dev/null +++ b/docs/tech-proposal-in-operator-audit.md @@ -0,0 +1,316 @@ +# Tech proposal: Audit and refactor `in` operator checks + +This document describes the audit and refactoring of `'prop' in obj` checks across the Cloudscape repositories, following an action item from the August 2024 Property Filter COE. + +Related links: + +* [COE: Property filter not creating filtering tokens](https://quip-amazon.com/YpPBAPz2BrqF/COE-Property-filter-not-creating-filtering-tokens) +* [SIM ticket: AWSUI-59006](https://issues.amazon.com/issues/AWSUI-59006) — Review current usage of the "in" property checks +* [PR with the fix](https://github.com/cloudscape-design/components/pull/2629) +* [PR introducing the bug](https://github.com/cloudscape-design/components/pull/2618) + +## The problem + +In August 2024, a refactoring changed the token type from `Token` (with `propertyKey`) to `InternalToken` (with `property`). The guard condition still checked for the old property: + +```ts +// BEFORE refactoring — worked correctly +interface Token { propertyKey?: string; operator: string; value: any; } +if (!('propertyKey' in newToken)) { return; } + +// AFTER refactoring — silently broken, always returns false +interface InternalToken { property: null | InternalFilteringProperty; operator: string; value: any; } +if (!('propertyKey' in newToken)) { return; } // ← 'propertyKey' no longer exists, but TS doesn't care +``` + +**TypeScript does not validate the `in` operator.** You can write `'banana' in obj` and it compiles fine. This means every `'prop' in obj` check in the codebase is vulnerable to the same silent breakage during refactoring. + +The fix was straightforward: + +```ts +// FIXED — direct property access, TypeScript validates this +if (!newToken.property) { return; } +``` + +## Audit results + +### Scope + +| Repository | Category 1 (actionable) | Category 2 (keep as-is) | Total | Status | +|------------|------------------------|------------------------|-------|--------| +| `cloudscape-design/components` | ~30 | ~10 | ~40 | ✅ Audited — type guards needed | +| `cloudscape-design/chart-components` | **0** | **12** | **12** | ✅ Audited — no action needed | + +--- + +### `components` repository + +We found **40 usages** of `'prop' in obj` across **20 files** in `src/`. They fall into two categories: + +#### Category 1: Union type discrimination (~30 usages) — CAN be made type-safe + +These check whether an object is one type or another in a union. For example, `InternalToken` has `operator` but `InternalTokenGroup` has `operation`: + +```ts +if ('operator' in tokenOrGroup) { + // it's a token +} +``` + +This is the standard TypeScript pattern for narrowing unions, but the string `'operator'` is **not validated** against the type. + +**Affected files and lines:** + +| Component | File | Code | GitHub link | +|-----------|------|------|-------------| +| property-filter | `internal.tsx:164` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/internal.tsx#L164) | +| property-filter | `token.tsx:74` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L74) | +| property-filter | `token.tsx:80` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L80) | +| property-filter | `token.tsx:87` | `'operator' in token` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L87) | +| property-filter | `utils.ts:146` | `'operator' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/utils.ts#L146) | +| property-filter | `utils.ts:150` | `'operator' in nestedTokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/utils.ts#L150) | +| property-filter | `controller.ts:49` | `'operator' in token` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/controller.ts#L49) | +| property-filter | `filter-options.ts:30` | `'options' in optionOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/filter-options.ts#L30) | +| autosuggest | `utils/utils.ts:8` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/utils/utils.ts#L8) | +| autosuggest | `options-controller.ts:182` | `'options' in optionOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/options-controller.ts#L182) | +| autosuggest | `autosuggest-option.tsx:111` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L111) | +| autosuggest | `autosuggest-option.tsx:112` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L112) | +| autosuggest | `autosuggest-option.tsx:113` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L113) | +| internal/option | `filter-options.ts:77` | `'options' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/components/option/utils/filter-options.ts#L77) | +| mixed-line-bar-chart | `utils.ts:103` | `'y' in series` | [link](https://github.com/cloudscape-design/components/tree/main/src/mixed-line-bar-chart/utils.ts#L103) | +| mixed-line-bar-chart | `utils.ts:109` | `'x' in series` | [link](https://github.com/cloudscape-design/components/tree/main/src/mixed-line-bar-chart/utils.ts#L109) | +| side-navigation | `util.tsx:53` | `'href' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/side-navigation/util.tsx#L53) | +| side-navigation | `util.tsx:60` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/side-navigation/util.tsx#L60) | +| top-navigation | `parts/utility.tsx:136` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/top-navigation/parts/utility.tsx#L136) | +| button-dropdown | `internal.tsx:184` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-dropdown/internal.tsx#L184) | +| button-dropdown | `internal.tsx:193` | `'badge' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-dropdown/internal.tsx#L193) | +| button-group | `item-element.tsx:106` | `'popoverFeedback' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-group/item-element.tsx#L106) | +| select | `check-option-value-field.ts:18` | `'options' in element` | [link](https://github.com/cloudscape-design/components/tree/main/src/select/utils/check-option-value-field.ts#L18) | +| flashbar | `collapsible-flashbar.tsx:217` | `'expandedIndex' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L217) | +| flashbar | `collapsible-flashbar.tsx:221` | `'expandedIndex' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L221) | +| flashbar | `non-collapsible-flashbar.tsx:127` | `'current' in transitionRootElement` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/non-collapsible-flashbar.tsx#L127) | +| flashbar | `collapsible-flashbar.tsx:307` | `'current' in transitionRootElement` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L307) | + +#### Category 2: Runtime / feature detection (~10 usages) — MUST stay as-is + +These check browser capabilities or cross-window duck typing where there is no TypeScript type to validate against. The `in` operator is the only correct approach here. + +| Component | File | Code | Why it must stay | GitHub link | +|-----------|------|------|------------------|-------------| +| internal/dom | `dom.ts:67-71` | `'nodeType' in target`, `'nodeName' in target`, `'parentNode' in target` | Cross-window duck typing (`instanceof` fails across iframes) | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L67) | +| internal/dom | `dom.ts:81` | `'style' in target` | Cross-window HTMLElement detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L81) | +| internal/dom | `dom.ts:93` | `'ownerSVGElement' in target` | Cross-window SVGElement detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L93) | +| focus-lock | `utils.ts:30` | `'checkVisibility' in element` | Browser API feature detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/components/focus-lock/utils.ts#L30) | +| tabs | `native-smooth-scroll-supported.ts:6` | `'scrollBehavior' in document.documentElement.style` | CSS feature detection | [link](https://github.com/cloudscape-design/components/tree/main/src/tabs/native-smooth-scroll-supported.ts#L6) | +| link | `internal.tsx:132` | `'button' in event` | MouseEvent vs KeyboardEvent | [link](https://github.com/cloudscape-design/components/tree/main/src/link/internal.tsx#L132) | +| navigable-group | `internal.tsx:136` | `'disabled' in element` | DOM element capability check | [link](https://github.com/cloudscape-design/components/tree/main/src/navigable-group/internal.tsx#L136) | +| app-layout | `runtime-drawer/index.tsx:90,94` | `'iconSvg' in runtimeHeaderAction` | Runtime plugin API | [link](https://github.com/cloudscape-design/components/tree/main/src/app-layout/runtime-drawer/index.tsx#L90) | +| app-layout | `use-app-layout.tsx:243` | `'payload' in message` | postMessage validation | [link](https://github.com/cloudscape-design/components/tree/main/src/app-layout/visual-refresh-toolbar/state/use-app-layout.tsx#L243) | +| property-filter | `internal.tsx:319` | `'keepOpenOnSelect' in option` | Runtime property not in TS type | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/internal.tsx#L319) | +| flashbar | `common.tsx:84` | `'id' in item` | Optional property existence check | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/common.tsx#L84) | +| test-utils | `code-editor/index.ts:56` | `'env' in editor` | External library duck typing | [link](https://github.com/cloudscape-design/components/tree/main/src/test-utils/dom/code-editor/index.ts#L56) | + +--- + +### `chart-components` repository + +We found **12 usages** of `'prop' in obj` across **6 files** in `src/` (plus 1 in test code). **All are Category 2** — no type guards are needed. + +#### Undocumented Highcharts properties (7 usages) + +Properties that exist at runtime but are intentionally absent from the Highcharts TypeScript definitions. Using `keyof` would be meaningless since the properties are not in the type. + +| Component | File | Code | Why it must stay | GitHub link | +|-----------|------|------|------------------|-------------| +| core/utils | `utils.ts:87` | `"symbol" in series` | `Highcharts.Series.symbol` is not in TS types but exists at runtime for scatter series markers | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L87) | +| core/utils | `utils.ts:90` | `"dashStyle" in series.options` | `dashStyle` exists on some Highcharts series types but not in the base `SeriesOptions` type | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L90) | +| core/utils | `utils.ts:387` | `"whiskers" in point` | `Highcharts.Point.whiskers` SVG element exists only on errorbar points at runtime, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L387) | +| chart-api/highlight | `chart-extra-highlight.ts:215` | `"plotLinesAndBands" in axis` | `Highcharts.Axis.plotLinesAndBands` array is explicitly not covered by TS types (comment in code) | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-highlight.ts#L215) | +| chart-api/tooltip | `chart-extra-tooltip.tsx:243` | `"tooltipPos" in point` | `Highcharts.Point.tooltipPos` tuple exists only on pie series points, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-tooltip.tsx#L243) | +| chart-api/index | `index.tsx:372` | `"colorCounter" in chart` | `Highcharts.Chart.colorCounter` is an internal property used for color assignment, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/index.tsx#L372) | +| internal/series-marker | `render-marker.tsx:102` | `"symbol" in series` | Same as first entry — undocumented `Highcharts.Series.symbol`, duplicate pattern in marker rendering | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/components/series-marker/render-marker.tsx#L102) | + +#### Highcharts complex external unions (3 usages) + +Properties being checked belong to Highcharts-defined union types (e.g., `SeriesOptionsType` is a union of 50+ series configuration types). These are external types not controlled by this codebase. + +| Component | File | Code | Why it must stay | GitHub link | +|-----------|------|------|------------------|-------------| +| core/utils | `utils.ts:218` | `"type" in series` | `Highcharts.SeriesOptionsType` is a union of 50+ series option types; `type` may or may not be present | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L218) | +| core/utils | `utils.ts:249` | `"xAxis" in item \|\| "yAxis" in item` | Duck-typing on `Highcharts.SeriesOptionsType \| Highcharts.PointOptionsType` — external union | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L249) | +| chart-api/nodata | `chart-extra-nodata.tsx:55-56` | `"data" in s.options` and `"y" in i` | Defensive check on Highcharts options; data items can be `number \| [number, number] \| { y: number } \| null` | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L55) | + +#### Browser API feature detection (1 usage) + +| Component | File | Code | Why it must stay | GitHub link | +|-----------|------|------|------------------|-------------| +| internal/utils/dom | `dom.ts:17` | `"scrollTo" in element` | `scrollTo` is not available in JSDOM test environments; classic feature detection pattern | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/utils/dom.ts#L17) | + +#### Test file usages (1 usage, not counted in primary audit) + +| Component | File | Code | Why it must stay | GitHub link | +|-----------|------|------|------------------|-------------| +| core/\_\_tests\_\_ | `common.tsx:68` | `"asymmetricMatch" in value` | Runtime Jest matcher detection — `asymmetricMatch` is a Jest convention property not in any TS type | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/__tests__/common.tsx#L68) | + +#### Notable cases + +- **`"symbol" in series`** (utils.ts:87, render-marker.tsx:102): The Highcharts `Series` type does not include `symbol` in its TS definition, but scatter series set it at runtime based on the marker configuration. The `in` check is the only way to safely access it without a type assertion. + +- **`"plotLinesAndBands" in axis`** (chart-extra-highlight.ts:215): The code even has an inline comment: *"The Highcharts `Axis.plotLinesAndBands` API is not covered with TS."* This is intentional — Highcharts considers this an internal API. + +- **`"tooltipPos" in point`** (chart-extra-tooltip.tsx:243): Pie chart points provide tooltip coordinates via an undocumented `tooltipPos` tuple. The code includes a fallback path for when this property is absent, making the `in` check a necessary defensive guard. + +- **`"colorCounter" in chart`** (index.tsx:372): This accesses a Highcharts internal used for automatic color assignment. It's part of a workaround for [a Highcharts bug](https://github.com/highcharts/highcharts/issues/23077). + +- **`"data" in s.options` and `"y" in i`** (chart-extra-nodata.tsx:55-56): Highcharts data arrays can contain `number`, `[number, number]`, `{ y: number }`, or `null`. The `"y" in i` check is the standard way to differentiate the object form from the primitive forms. + +#### Additional patterns found in chart-components + +| Pattern | Count | Details | +|---------|-------|---------| +| `for...in` loops | 0 | None found | +| `Object.keys()` | 1 | `src/internal/base-component/get-data-attributes.ts` — iterating `data-*` attributes from props | +| `Object.entries()` | 1 | `src/core/chart-api/chart-extra-navigation.ts` — iterating element attributes for `setAttribute` | +| `hasOwnProperty` | 0 | None found | + +--- + +## Proposed solution: type guard functions with `keyof` + +Replace each inline `'prop' in obj` check (Category 1, components repo only) with a **type guard function** that validates the property name at compile time: + +```ts +// BEFORE — 'operator' is an unchecked string, vulnerable to refactoring +if ('operator' in tokenOrGroup) { + // tokenOrGroup is narrowed to InternalToken +} + +// AFTER — 'operator' is validated against InternalToken via keyof +function isInternalToken( + t: InternalToken | InternalTokenGroup +): t is InternalToken { + const key: keyof InternalToken = 'operator'; // ← TypeScript validates this! + return key in t; +} + +if (isInternalToken(tokenOrGroup)) { + // tokenOrGroup is narrowed to InternalToken +} +``` + +**Why this works:** `keyof InternalToken` restricts the value to actual properties of `InternalToken`. If someone renames `operator` to `op`, the line `const key: keyof InternalToken = 'operator'` immediately produces a TypeScript error. + +### Type guards to create + +Each union type gets one or two type guard functions. All checks for that union are replaced with the same function call: + +| Type guard | Union | Discriminant | Replaces | +|-----------|-------|-------------|----------| +| `isInternalToken()` | `InternalToken \| InternalTokenGroup` | `operator` | 7 checks in property-filter | +| `isInternalTokenGroup()` | `InternalToken \| InternalTokenGroup` | `operation` | 2 checks in property-filter | +| `isOptionGroup()` | `Option \| OptionGroup` | `options` | 3 checks in property-filter, internal/option, select | +| `isAutosuggestGroup()` | `AutosuggestItem` union | `type` | 4 checks in autosuggest | +| `isAutosuggestOptionGroup()` | option vs group | `options` | 1 check in autosuggest | +| `isThresholdWithY()` | threshold series | `y` | 1 check in mixed-line-bar-chart | +| `isThresholdWithX()` | threshold series | `x` | 1 check in mixed-line-bar-chart | +| `isSideNavLink()` | SideNavigation item union | `href` | 1 check in side-navigation | +| `isSideNavSection()` | SideNavigation item union | `items` | 1 check in side-navigation | +| `isMenuDropdown()` | TopNavigation utility union | `items` | 1 check in top-navigation | +| `isItemGroup()` | ButtonDropdown item union | `items` | 1 check in button-dropdown | +| `isStackableItem()` | `StackableItem \| MessageDefinition` | `expandedIndex` | 2 checks in flashbar | +| `isReactRef()` | `HTMLElement \| React.RefObject` | `current` | 2 checks in flashbar | + +### Where type guards live + +Type guard functions will be co-located with the types they guard — typically in the same file as the interface definition or in a dedicated `utils.ts` within the component folder. For types shared across components (e.g., `OptionGroup` used in property-filter, internal/option, and select), the guard will live next to the shared type definition and be imported where needed. + +### What stays as-is + +The ~10 runtime/feature-detection checks in the components repo (Category 2) and all 12 checks in chart-components cannot use `keyof` because we don't own the types. These stay as inline `in` checks with an `eslint-disable` comment explaining why. + +Notable cases in components Category 2: + +- **`'keepOpenOnSelect' in option`** (property-filter): This property is intentionally absent from the public type definition — it's an internal runtime extension added programmatically to certain autosuggest options. Adding it to the type would expose it in the public API. Keeping the `in` check with an eslint-disable comment is the right approach. +- **`'id' in item`** (flashbar): `id` is an optional property on `FlashbarProps.MessageDefinition`. The `in` check tests for runtime key existence (whether the user provided the key at all), which is subtly different from `item.id !== undefined`. Both would work in practice, but we keep `in` here for semantic correctness. + +## Test coverage + +The COE action item asks to both refactor `in` checks and ensure test coverage. Our approach: + +1. Existing component tests that exercise union-type code paths (e.g., property filter with token groups, flashbar with stacked items) already provide integration-level coverage for the logic that depends on these checks. +2. We will verify that each `in` check code path is reachable from existing tests. Where coverage gaps are found, we will document them for follow-up but not block the refactoring on adding new integration tests. +3. The real testing opportunity is with the cases where a guard function **cannot** be introduced (e.g., `'keepOpenOnSelect' in option`, `'id' in item`). There, component-level tests that exercise both branches can fail conveniently if unsafe changes are made to the concerned code. + +## Future prevention: ESLint rule + +To prevent new raw `in` checks from being introduced, add an ESLint rule: + +```js +// eslint.config.mjs +{ + 'no-restricted-syntax': ['error', { + selector: 'BinaryExpression[operator="in"]', + message: 'Do not use the "in" operator directly. Use a typed type guard function instead.' + }] +} +``` + +The Category 2 usages get `// eslint-disable-next-line no-restricted-syntax` with an explanation. + +Note: This rule only catches the binary expression `'prop' in obj`, not `for...in` loops (which use `ForInStatement` in the AST, a different node type). + +### What this gives us + +| Scenario | Today | After this change | +|----------|-------|-------------------| +| Someone renames a property during refactoring | ❌ Silent failure at runtime (the COE bug) | ✅ TypeScript compile error in the type guard | +| Someone writes a new `'prop' in obj` check | ❌ No warning | ✅ ESLint error, must use a type guard | +| Browser feature detection | ✅ Works | ✅ Still works (explicit eslint-disable) | + +## Effort and risk + +**Estimated effort:** ~3-4 days of implementation + code review for the components repo. No code changes needed for chart-components. + +**Risk:** Low. Type guard functions are pure wrappers around the existing `in` logic — runtime behavior is identical. No public API, visual, or behavioral changes. If any issue is found, type guards can be inlined back to raw `in` checks with a simple find-and-replace. + +**Why now?** While no similar bug has recurred since the August 2024 COE, the risk is latent — any future refactoring of a discriminant property will silently break the corresponding `in` check. The type guard approach addresses this with minimal effort and also introduces an ESLint rule that prevents the pattern from proliferating further. Given that we found 40 usages across 20 files (and this number will only grow), addressing it now is cheaper than addressing it later. + +## Alternatives considered + +### Direct property access (`obj.prop`) + +Replace `'prop' in obj` with `obj.prop !== undefined`. + +- ✅ TypeScript validates property names +- ❌ Only works when the property exists on all types in the union (otherwise it's a type error) +- ❌ Doesn't narrow union types +- Only applicable to ~3 of the 40 cases + +### Discriminant tag property (`kind: 'token' | 'group'`) + +Add an explicit `kind` field to all union types. + +- ✅ Gold standard for discriminated unions +- ❌ Requires changing internal interfaces across many components +- ❌ Much larger scope (~2-3 weeks vs ~1 week) +- Could be done as a future improvement + +Adding a `kind` discriminant to e.g. `InternalToken | InternalTokenGroup` would require changing every call site that **creates** these objects (~15+ locations in property-filter alone), updating all test fixtures, and modifying serialization/deserialization logic. The type guard approach achieves the same compile-time safety with changes only at the ~30 **consumption** sites, not the creation sites. + +### Why not a `@typescript-eslint` rule? + +`@typescript-eslint` does not have a built-in rule that validates the left-hand side of `in` against the type of the right-hand side. Writing a custom type-aware ESLint rule is significantly more complex (requires the TypeScript type-checker integration) and not worth the effort when `no-restricted-syntax` + type guards already cover the problem completely. + +## Feedback + +Notes: + +* ... + +Voting: + +| Reviewer | Vote | Comments | +|----------|------|----------| +| | | | +| | | | +| | | | \ No newline at end of file diff --git a/src/core/chart-api/chart-extra-pointer.tsx b/src/core/chart-api/chart-extra-pointer.tsx index 1e297e18..c80d2f55 100644 --- a/src/core/chart-api/chart-extra-pointer.tsx +++ b/src/core/chart-api/chart-extra-pointer.tsx @@ -8,8 +8,6 @@ import { DebouncedCall } from "../../internal/utils/utils"; import { isPointVisible } from "../utils"; import { ChartExtraContext } from "./chart-extra-context"; -import styles from "../styles.css.js"; - const HOVER_LOST_DELAY = 25; export interface ChartExtraPointerHandlers { @@ -189,9 +187,8 @@ export class ChartExtraPointer { private applyCursorStyle = () => { const container = this.context.chart().container; if (container) { - const isHovered = !!(this.hoveredPoint || this.hoveredGroup); - container.classList.toggle(styles["cursor-pointer"], isHovered); - container.classList.toggle(styles["cursor-default"], !isHovered); + const value = this.hoveredPoint || this.hoveredGroup ? "pointer" : "default"; + container.style.cursor = value; } }; } diff --git a/src/core/chart-api/chart-extra-tooltip.tsx b/src/core/chart-api/chart-extra-tooltip.tsx index 97dd654a..83e45c54 100644 --- a/src/core/chart-api/chart-extra-tooltip.tsx +++ b/src/core/chart-api/chart-extra-tooltip.tsx @@ -168,7 +168,7 @@ export class ChartExtraTooltip extends AsyncStore { }; private get commonTrackAttrs() { - return { fill: "transparent", zIndex: -1, style: "pointer-events:none", direction: this.direction }; + return { fill: "transparent", zIndex: -1, "pointer-events": "none", direction: this.direction }; } // We compute the direction from the chart container and then explicitly set it to the track diff --git a/src/core/styles.scss b/src/core/styles.scss index 075d7171..d9c5f23d 100644 --- a/src/core/styles.scss +++ b/src/core/styles.scss @@ -131,11 +131,3 @@ $side-legend-max-inline-size: 30%; // We hide the native focus outline to render a custom one around the chart plot instead. outline: none; } - -.cursor-pointer { - cursor: pointer; -} - -.cursor-default { - cursor: default; -} diff --git a/src/internal/chart-styles.ts b/src/internal/chart-styles.ts index 38549a41..ce031254 100644 --- a/src/internal/chart-styles.ts +++ b/src/internal/chart-styles.ts @@ -148,7 +148,7 @@ export const navigationFocusOutline = { export const cursorLine = { fill: colorChartsLineTick, zIndex: 5, - style: "pointer-events: none", + "pointer-events": "none", }; export const focusOutlineOffsets = { diff --git a/src/internal/components/series-marker/render-marker.tsx b/src/internal/components/series-marker/render-marker.tsx index bb7d7799..2eb95f44 100644 --- a/src/internal/components/series-marker/render-marker.tsx +++ b/src/internal/components/series-marker/render-marker.tsx @@ -80,7 +80,7 @@ function getDefaultPointProps(point: Highcharts.Point, selected: boolean, classN const pointStyle: Highcharts.SVGAttributes = { zIndex: selected ? 6 : 5, opacity: 1, - style: "pointer-events: none", + "pointer-events": "none", class: className, "stroke-width": 2, stroke: selected ? colorTextBodyDefault : point.color, @@ -92,7 +92,7 @@ function getDefaultPointProps(point: Highcharts.Point, selected: boolean, classN stroke: point.color, fill: "transparent", opacity: 0.4, - style: "pointer-events: none", + "pointer-events": "none", r: 8, }; return { size, pointStyle, haloStyle }; From e7107ae5c58594e611cfd55c1b16fed2d2845a88 Mon Sep 17 00:00:00 2001 From: Philipp Schneider <115801132+Who-is-PS@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:12:32 +0100 Subject: [PATCH 3/4] Delete docs/in-operator-review-AWSUI-59006.md --- docs/in-operator-review-AWSUI-59006.md | 279 ------------------------- 1 file changed, 279 deletions(-) delete mode 100644 docs/in-operator-review-AWSUI-59006.md diff --git a/docs/in-operator-review-AWSUI-59006.md b/docs/in-operator-review-AWSUI-59006.md deleted file mode 100644 index e3df79e3..00000000 --- a/docs/in-operator-review-AWSUI-59006.md +++ /dev/null @@ -1,279 +0,0 @@ -# `in` Operator Audit Findings — AWSUI-59006 - -## Repository: `cloudscape-design/chart-components` - -**Audit date:** 2025-02-19 -**GitHub base URL:** https://github.com/cloudscape-design/chart-components/tree/main/src - -Related: [Tech proposal](./tech-proposal-in-operator-audit.md) - ---- - -## Summary - -| Metric | Count | -|--------|-------| -| Total `'prop' in obj` usages (source) | 12 | -| Category 1 — Union type discrimination | **0** | -| Category 2 — Runtime/feature detection | **12** | -| Test file usages | 1 | -| Type guards to create | **0** | -| Actionable items | **0** | - -**All 12 usages are Category 2 and MUST stay as-is.** No code changes required. - ---- - -## Detailed findings - -### Usage #1: `"symbol" in series` - -**File:** [`src/core/utils.ts:92`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L92) -**Function:** `getSeriesMarkerType()` -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `Highcharts.Series.symbol` is not in the TS type definition but exists at runtime for scatter series markers. - -```ts -// src/core/utils.ts, lines 88–96 -export function getSeriesMarkerType(series?: Highcharts.Series): ChartSeriesMarkerType { - if (!series) { - return "large-square"; - } - const seriesSymbol = "symbol" in series && typeof series.symbol === "string" ? series.symbol : "circle"; - // In Highcharts, dashStyle supports different types of dashes - if ("dashStyle" in series.options && series.options.dashStyle && series.options.dashStyle !== "Solid") { - return "dashed"; - } -``` - ---- - -### Usage #2: `"dashStyle" in series.options` - -**File:** [`src/core/utils.ts:94`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L94) -**Function:** `getSeriesMarkerType()` -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `dashStyle` exists on some Highcharts series types but not in the base `SeriesOptions` type. - -```ts -// src/core/utils.ts, lines 94–96 - if ("dashStyle" in series.options && series.options.dashStyle && series.options.dashStyle !== "Solid") { - return "dashed"; - } -``` - ---- - -### Usage #3: `"type" in series` - -**File:** [`src/core/utils.ts:193`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L193) -**Function:** `getVisibleLegendItems()` -**Category:** 2 — Highcharts complex external union -**Why it must stay:** `Highcharts.SeriesOptionsType` is a union of 50+ series option types; `type` may or may not be explicitly present. - -```ts -// src/core/utils.ts, lines 191–196 - options.series?.forEach((series) => { - if (!("type" in series)) { - return; - } - - // The pie series is not shown in the legend. Instead, we show pie segments (points). -``` - ---- - -### Usage #4: `"xAxis" in item || "yAxis" in item` - -**File:** [`src/core/utils.ts:208`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L208) -**Function:** `isSecondaryLegendItem()` -**Category:** 2 — Highcharts complex external union -**Why it must stay:** Duck-typing on `Highcharts.SeriesOptionsType | Highcharts.PointOptionsType` — external union not controlled by this codebase. - -```ts -// src/core/utils.ts, lines 206–211 - // Only items that have an associated axis can be considered secondary - if (item && typeof item === "object" && ("xAxis" in item || "yAxis" in item)) { - const axisRef = isInverted ? (item.xAxis ?? 0) : (item.yAxis ?? 0); - // An axis reference can be an index into the axes array or an explicitly passed id - const valueAxis = typeof axisRef === "number" ? valueAxes[axisRef] : valueAxes.find((a) => a.id === axisRef); - return valueAxis?.opposite ?? false; -``` - ---- - -### Usage #5: `"whiskers" in point` - -**File:** [`src/core/utils.ts:275`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L275) -**Function:** `getErrorBarPointRect()` -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `Highcharts.Point.whiskers` SVG element exists only on errorbar points at runtime, not in TS types. - -```ts -// src/core/utils.ts, lines 272–278 -function getErrorBarPointRect(point: Highcharts.Point): Rect { - const chart = point.series.chart; - if ("whiskers" in point) { - return getChartRect((point.whiskers as Highcharts.SVGElement).getBBox(), chart, true); - } - return getPointRectFromCoordinates(point); -} -``` - ---- - -### Usage #6: `"plotLinesAndBands" in axis` - -**File:** [`src/core/chart-api/chart-extra-highlight.ts:187`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-highlight.ts#L187) -**Function:** `iteratePlotLines()` -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `Highcharts.Axis.plotLinesAndBands` array is explicitly not covered by TS types. The code has an inline comment: *"The Highcharts `Axis.plotLinesAndBands` API is not covered with TS."* - -```ts -// src/core/chart-api/chart-extra-highlight.ts, lines 185–193 -function iteratePlotLines(chart: Highcharts.Chart, cb: (lineId: string, line: Highcharts.PlotLineOrBand) => void) { - chart.axes?.forEach((axis) => { - // The Highcharts `Axis.plotLinesAndBands` API is not covered with TS. - if ("plotLinesAndBands" in axis && Array.isArray(axis.plotLinesAndBands)) { - axis.plotLinesAndBands.forEach((line: Highcharts.PlotLineOrBand) => { - if (line.options.id) { - cb(line.options.id, line); - } - }); -``` - ---- - -### Usage #7: `"tooltipPos" in point` - -**File:** [`src/core/chart-api/chart-extra-tooltip.tsx:208`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-tooltip.tsx#L208) -**Function:** `getPieChartTargetPlacement()` -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `Highcharts.Point.tooltipPos` tuple exists only on pie series points at runtime, not in TS types. The code includes a fallback path for when this property is absent. - -```ts -// src/core/chart-api/chart-extra-tooltip.tsx, lines 205–213 -function getPieChartTargetPlacement(point: Highcharts.Point): Rect { - // The pie series segments do not provide plotX, plotY to compute the tooltip placement. - // Instead, there is a `tooltipPos` tuple, which is not covered by TS. - // See: https://github.com/highcharts/highcharts/issues/23118. - if ("tooltipPos" in point && Array.isArray(point.tooltipPos)) { - return safeRect({ x: point.tooltipPos[0], y: point.tooltipPos[1], width: 0, height: 0 }); - } - // We use the alternative, middle, tooltip placement as a fallback - return getPieMiddlePlacement(point); -``` - ---- - -### Usage #8: `"data" in s.options` - -**File:** [`src/core/chart-api/chart-extra-nodata.tsx:55`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L55) -**Function:** `findAllSeriesWithData()` -**Category:** 2 — Highcharts complex external union -**Why it must stay:** Defensive check on `Highcharts.SeriesOptions` — `data` property may or may not be present depending on series type. - -```ts -// src/core/chart-api/chart-extra-nodata.tsx, lines 53–57 -function findAllSeriesWithData(chart: Highcharts.Chart) { - return getChartSeries(chart.series).filter((s) => { - const data = "data" in s.options && s.options.data && Array.isArray(s.options.data) ? s.options.data : []; - return data.some((i) => i !== null && (typeof i === "object" && "y" in i ? i.y !== null : true)); - }); -``` - ---- - -### Usage #9: `"y" in i` - -**File:** [`src/core/chart-api/chart-extra-nodata.tsx:56`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L56) -**Function:** `findAllSeriesWithData()` -**Category:** 2 — Highcharts complex external union -**Why it must stay:** Differentiating Highcharts data formats — data items can be `number | [number, number] | { y: number } | null`. - -```ts -// src/core/chart-api/chart-extra-nodata.tsx, line 56 - return data.some((i) => i !== null && (typeof i === "object" && "y" in i ? i.y !== null : true)); -``` - ---- - -### Usage #10: `"colorCounter" in chart` - -**File:** [`src/core/chart-api/index.tsx:290`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/index.tsx#L290) -**Function:** `ChartAPI.resetColorCounter()` (private method) -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** `Highcharts.Chart.colorCounter` is an internal property used for color assignment. Part of a workaround for [Highcharts bug #23077](https://github.com/highcharts/highcharts/issues/23077). - -```ts -// src/core/chart-api/index.tsx, lines 287–292 - private resetColorCounter() { - const chart = this.context.chart(); - if ("colorCounter" in chart && typeof chart.colorCounter === "number") { - chart.colorCounter = getChartSeries(chart.series).length; - } - } -``` - ---- - -### Usage #11: `"scrollTo" in element` - -**File:** [`src/internal/utils/dom.ts:18`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/utils/dom.ts#L18) -**Function:** `scrollIntoViewNearestContainer()` -**Category:** 2 — Browser API feature detection -**Why it must stay:** `scrollTo` is not available in JSDOM test environments. Classic feature detection pattern. - -```ts -// src/internal/utils/dom.ts, lines 16–20 - // Scroll methods (scrollTo, scrollIntoView, etc) are not supported in test environments like JSDOM. - if (!("scrollTo" in element)) { - return; - } -``` - ---- - -### Usage #12: `"symbol" in series` (duplicate) - -**File:** [`src/internal/components/series-marker/render-marker.tsx:82`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/components/series-marker/render-marker.tsx#L82) -**Function:** `getSeriesMarkerType()` (local to render-marker) -**Category:** 2 — Undocumented Highcharts property -**Why it must stay:** Same pattern as Usage #1 — undocumented `Highcharts.Series.symbol`, duplicate in a separate marker rendering module. - -```ts -// src/internal/components/series-marker/render-marker.tsx, lines 80–84 -function getSeriesMarkerType(series: Highcharts.Series): ChartSeriesMarkerType { - const seriesSymbol = "symbol" in series && typeof series.symbol === "string" ? series.symbol : "circle"; - if (series.type === "scatter") { - switch (seriesSymbol) { -``` - ---- - -### Test file usage: `"asymmetricMatch" in value` - -**File:** [`src/core/__tests__/common.tsx:72`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/__tests__/common.tsx#L72) -**Function:** `isJestMatcher()` (local helper in `objectContainingDeep`) -**Category:** 2 — Runtime Jest matcher detection -**Why it must stay:** `asymmetricMatch` is a Jest convention property not in any TS type. - -```ts -// src/core/__tests__/common.tsx, lines 70–74 -export function objectContainingDeep(root: object) { - function isJestMatcher(value: object): boolean { - return "asymmetricMatch" in value; - } - function transformLevel(obj: object) { -``` - ---- - -## Additional patterns found - -| Pattern | Count | Location | -|---------|-------|----------| -| `for...in` loops | 0 | — | -| `Object.keys()` | 1 | [`src/internal/base-component/get-data-attributes.ts`](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/base-component/get-data-attributes.ts) — iterating `data-*` attributes from props | -| `Object.entries()` | 1 | [`src/core/chart-api/chart-extra-navigation.ts`](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-navigation.ts) — iterating element attributes for `setAttribute` | -| `hasOwnProperty` | 0 | — | \ No newline at end of file From fac89034e18409a8e3995444667f97a7cc96bc52 Mon Sep 17 00:00:00 2001 From: Philipp Schneider <115801132+Who-is-PS@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:14:26 +0100 Subject: [PATCH 4/4] Delete docs/tech-proposal-in-operator-audit.md --- docs/tech-proposal-in-operator-audit.md | 316 ------------------------ 1 file changed, 316 deletions(-) delete mode 100644 docs/tech-proposal-in-operator-audit.md diff --git a/docs/tech-proposal-in-operator-audit.md b/docs/tech-proposal-in-operator-audit.md deleted file mode 100644 index d44eafb1..00000000 --- a/docs/tech-proposal-in-operator-audit.md +++ /dev/null @@ -1,316 +0,0 @@ -# Tech proposal: Audit and refactor `in` operator checks - -This document describes the audit and refactoring of `'prop' in obj` checks across the Cloudscape repositories, following an action item from the August 2024 Property Filter COE. - -Related links: - -* [COE: Property filter not creating filtering tokens](https://quip-amazon.com/YpPBAPz2BrqF/COE-Property-filter-not-creating-filtering-tokens) -* [SIM ticket: AWSUI-59006](https://issues.amazon.com/issues/AWSUI-59006) — Review current usage of the "in" property checks -* [PR with the fix](https://github.com/cloudscape-design/components/pull/2629) -* [PR introducing the bug](https://github.com/cloudscape-design/components/pull/2618) - -## The problem - -In August 2024, a refactoring changed the token type from `Token` (with `propertyKey`) to `InternalToken` (with `property`). The guard condition still checked for the old property: - -```ts -// BEFORE refactoring — worked correctly -interface Token { propertyKey?: string; operator: string; value: any; } -if (!('propertyKey' in newToken)) { return; } - -// AFTER refactoring — silently broken, always returns false -interface InternalToken { property: null | InternalFilteringProperty; operator: string; value: any; } -if (!('propertyKey' in newToken)) { return; } // ← 'propertyKey' no longer exists, but TS doesn't care -``` - -**TypeScript does not validate the `in` operator.** You can write `'banana' in obj` and it compiles fine. This means every `'prop' in obj` check in the codebase is vulnerable to the same silent breakage during refactoring. - -The fix was straightforward: - -```ts -// FIXED — direct property access, TypeScript validates this -if (!newToken.property) { return; } -``` - -## Audit results - -### Scope - -| Repository | Category 1 (actionable) | Category 2 (keep as-is) | Total | Status | -|------------|------------------------|------------------------|-------|--------| -| `cloudscape-design/components` | ~30 | ~10 | ~40 | ✅ Audited — type guards needed | -| `cloudscape-design/chart-components` | **0** | **12** | **12** | ✅ Audited — no action needed | - ---- - -### `components` repository - -We found **40 usages** of `'prop' in obj` across **20 files** in `src/`. They fall into two categories: - -#### Category 1: Union type discrimination (~30 usages) — CAN be made type-safe - -These check whether an object is one type or another in a union. For example, `InternalToken` has `operator` but `InternalTokenGroup` has `operation`: - -```ts -if ('operator' in tokenOrGroup) { - // it's a token -} -``` - -This is the standard TypeScript pattern for narrowing unions, but the string `'operator'` is **not validated** against the type. - -**Affected files and lines:** - -| Component | File | Code | GitHub link | -|-----------|------|------|-------------| -| property-filter | `internal.tsx:164` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/internal.tsx#L164) | -| property-filter | `token.tsx:74` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L74) | -| property-filter | `token.tsx:80` | `'operation' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L80) | -| property-filter | `token.tsx:87` | `'operator' in token` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/token.tsx#L87) | -| property-filter | `utils.ts:146` | `'operator' in tokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/utils.ts#L146) | -| property-filter | `utils.ts:150` | `'operator' in nestedTokenOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/utils.ts#L150) | -| property-filter | `controller.ts:49` | `'operator' in token` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/controller.ts#L49) | -| property-filter | `filter-options.ts:30` | `'options' in optionOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/filter-options.ts#L30) | -| autosuggest | `utils/utils.ts:8` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/utils/utils.ts#L8) | -| autosuggest | `options-controller.ts:182` | `'options' in optionOrGroup` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/options-controller.ts#L182) | -| autosuggest | `autosuggest-option.tsx:111` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L111) | -| autosuggest | `autosuggest-option.tsx:112` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L112) | -| autosuggest | `autosuggest-option.tsx:113` | `'type' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/autosuggest/autosuggest-option.tsx#L113) | -| internal/option | `filter-options.ts:77` | `'options' in option` | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/components/option/utils/filter-options.ts#L77) | -| mixed-line-bar-chart | `utils.ts:103` | `'y' in series` | [link](https://github.com/cloudscape-design/components/tree/main/src/mixed-line-bar-chart/utils.ts#L103) | -| mixed-line-bar-chart | `utils.ts:109` | `'x' in series` | [link](https://github.com/cloudscape-design/components/tree/main/src/mixed-line-bar-chart/utils.ts#L109) | -| side-navigation | `util.tsx:53` | `'href' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/side-navigation/util.tsx#L53) | -| side-navigation | `util.tsx:60` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/side-navigation/util.tsx#L60) | -| top-navigation | `parts/utility.tsx:136` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/top-navigation/parts/utility.tsx#L136) | -| button-dropdown | `internal.tsx:184` | `'items' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-dropdown/internal.tsx#L184) | -| button-dropdown | `internal.tsx:193` | `'badge' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-dropdown/internal.tsx#L193) | -| button-group | `item-element.tsx:106` | `'popoverFeedback' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/button-group/item-element.tsx#L106) | -| select | `check-option-value-field.ts:18` | `'options' in element` | [link](https://github.com/cloudscape-design/components/tree/main/src/select/utils/check-option-value-field.ts#L18) | -| flashbar | `collapsible-flashbar.tsx:217` | `'expandedIndex' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L217) | -| flashbar | `collapsible-flashbar.tsx:221` | `'expandedIndex' in item` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L221) | -| flashbar | `non-collapsible-flashbar.tsx:127` | `'current' in transitionRootElement` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/non-collapsible-flashbar.tsx#L127) | -| flashbar | `collapsible-flashbar.tsx:307` | `'current' in transitionRootElement` | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/collapsible-flashbar.tsx#L307) | - -#### Category 2: Runtime / feature detection (~10 usages) — MUST stay as-is - -These check browser capabilities or cross-window duck typing where there is no TypeScript type to validate against. The `in` operator is the only correct approach here. - -| Component | File | Code | Why it must stay | GitHub link | -|-----------|------|------|------------------|-------------| -| internal/dom | `dom.ts:67-71` | `'nodeType' in target`, `'nodeName' in target`, `'parentNode' in target` | Cross-window duck typing (`instanceof` fails across iframes) | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L67) | -| internal/dom | `dom.ts:81` | `'style' in target` | Cross-window HTMLElement detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L81) | -| internal/dom | `dom.ts:93` | `'ownerSVGElement' in target` | Cross-window SVGElement detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/utils/dom.ts#L93) | -| focus-lock | `utils.ts:30` | `'checkVisibility' in element` | Browser API feature detection | [link](https://github.com/cloudscape-design/components/tree/main/src/internal/components/focus-lock/utils.ts#L30) | -| tabs | `native-smooth-scroll-supported.ts:6` | `'scrollBehavior' in document.documentElement.style` | CSS feature detection | [link](https://github.com/cloudscape-design/components/tree/main/src/tabs/native-smooth-scroll-supported.ts#L6) | -| link | `internal.tsx:132` | `'button' in event` | MouseEvent vs KeyboardEvent | [link](https://github.com/cloudscape-design/components/tree/main/src/link/internal.tsx#L132) | -| navigable-group | `internal.tsx:136` | `'disabled' in element` | DOM element capability check | [link](https://github.com/cloudscape-design/components/tree/main/src/navigable-group/internal.tsx#L136) | -| app-layout | `runtime-drawer/index.tsx:90,94` | `'iconSvg' in runtimeHeaderAction` | Runtime plugin API | [link](https://github.com/cloudscape-design/components/tree/main/src/app-layout/runtime-drawer/index.tsx#L90) | -| app-layout | `use-app-layout.tsx:243` | `'payload' in message` | postMessage validation | [link](https://github.com/cloudscape-design/components/tree/main/src/app-layout/visual-refresh-toolbar/state/use-app-layout.tsx#L243) | -| property-filter | `internal.tsx:319` | `'keepOpenOnSelect' in option` | Runtime property not in TS type | [link](https://github.com/cloudscape-design/components/tree/main/src/property-filter/internal.tsx#L319) | -| flashbar | `common.tsx:84` | `'id' in item` | Optional property existence check | [link](https://github.com/cloudscape-design/components/tree/main/src/flashbar/common.tsx#L84) | -| test-utils | `code-editor/index.ts:56` | `'env' in editor` | External library duck typing | [link](https://github.com/cloudscape-design/components/tree/main/src/test-utils/dom/code-editor/index.ts#L56) | - ---- - -### `chart-components` repository - -We found **12 usages** of `'prop' in obj` across **6 files** in `src/` (plus 1 in test code). **All are Category 2** — no type guards are needed. - -#### Undocumented Highcharts properties (7 usages) - -Properties that exist at runtime but are intentionally absent from the Highcharts TypeScript definitions. Using `keyof` would be meaningless since the properties are not in the type. - -| Component | File | Code | Why it must stay | GitHub link | -|-----------|------|------|------------------|-------------| -| core/utils | `utils.ts:87` | `"symbol" in series` | `Highcharts.Series.symbol` is not in TS types but exists at runtime for scatter series markers | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L87) | -| core/utils | `utils.ts:90` | `"dashStyle" in series.options` | `dashStyle` exists on some Highcharts series types but not in the base `SeriesOptions` type | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L90) | -| core/utils | `utils.ts:387` | `"whiskers" in point` | `Highcharts.Point.whiskers` SVG element exists only on errorbar points at runtime, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L387) | -| chart-api/highlight | `chart-extra-highlight.ts:215` | `"plotLinesAndBands" in axis` | `Highcharts.Axis.plotLinesAndBands` array is explicitly not covered by TS types (comment in code) | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-highlight.ts#L215) | -| chart-api/tooltip | `chart-extra-tooltip.tsx:243` | `"tooltipPos" in point` | `Highcharts.Point.tooltipPos` tuple exists only on pie series points, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-tooltip.tsx#L243) | -| chart-api/index | `index.tsx:372` | `"colorCounter" in chart` | `Highcharts.Chart.colorCounter` is an internal property used for color assignment, not in TS types | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/index.tsx#L372) | -| internal/series-marker | `render-marker.tsx:102` | `"symbol" in series` | Same as first entry — undocumented `Highcharts.Series.symbol`, duplicate pattern in marker rendering | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/components/series-marker/render-marker.tsx#L102) | - -#### Highcharts complex external unions (3 usages) - -Properties being checked belong to Highcharts-defined union types (e.g., `SeriesOptionsType` is a union of 50+ series configuration types). These are external types not controlled by this codebase. - -| Component | File | Code | Why it must stay | GitHub link | -|-----------|------|------|------------------|-------------| -| core/utils | `utils.ts:218` | `"type" in series` | `Highcharts.SeriesOptionsType` is a union of 50+ series option types; `type` may or may not be present | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L218) | -| core/utils | `utils.ts:249` | `"xAxis" in item \|\| "yAxis" in item` | Duck-typing on `Highcharts.SeriesOptionsType \| Highcharts.PointOptionsType` — external union | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/utils.ts#L249) | -| chart-api/nodata | `chart-extra-nodata.tsx:55-56` | `"data" in s.options` and `"y" in i` | Defensive check on Highcharts options; data items can be `number \| [number, number] \| { y: number } \| null` | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/chart-api/chart-extra-nodata.tsx#L55) | - -#### Browser API feature detection (1 usage) - -| Component | File | Code | Why it must stay | GitHub link | -|-----------|------|------|------------------|-------------| -| internal/utils/dom | `dom.ts:17` | `"scrollTo" in element` | `scrollTo` is not available in JSDOM test environments; classic feature detection pattern | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/internal/utils/dom.ts#L17) | - -#### Test file usages (1 usage, not counted in primary audit) - -| Component | File | Code | Why it must stay | GitHub link | -|-----------|------|------|------------------|-------------| -| core/\_\_tests\_\_ | `common.tsx:68` | `"asymmetricMatch" in value` | Runtime Jest matcher detection — `asymmetricMatch` is a Jest convention property not in any TS type | [link](https://github.com/cloudscape-design/chart-components/tree/main/src/core/__tests__/common.tsx#L68) | - -#### Notable cases - -- **`"symbol" in series`** (utils.ts:87, render-marker.tsx:102): The Highcharts `Series` type does not include `symbol` in its TS definition, but scatter series set it at runtime based on the marker configuration. The `in` check is the only way to safely access it without a type assertion. - -- **`"plotLinesAndBands" in axis`** (chart-extra-highlight.ts:215): The code even has an inline comment: *"The Highcharts `Axis.plotLinesAndBands` API is not covered with TS."* This is intentional — Highcharts considers this an internal API. - -- **`"tooltipPos" in point`** (chart-extra-tooltip.tsx:243): Pie chart points provide tooltip coordinates via an undocumented `tooltipPos` tuple. The code includes a fallback path for when this property is absent, making the `in` check a necessary defensive guard. - -- **`"colorCounter" in chart`** (index.tsx:372): This accesses a Highcharts internal used for automatic color assignment. It's part of a workaround for [a Highcharts bug](https://github.com/highcharts/highcharts/issues/23077). - -- **`"data" in s.options` and `"y" in i`** (chart-extra-nodata.tsx:55-56): Highcharts data arrays can contain `number`, `[number, number]`, `{ y: number }`, or `null`. The `"y" in i` check is the standard way to differentiate the object form from the primitive forms. - -#### Additional patterns found in chart-components - -| Pattern | Count | Details | -|---------|-------|---------| -| `for...in` loops | 0 | None found | -| `Object.keys()` | 1 | `src/internal/base-component/get-data-attributes.ts` — iterating `data-*` attributes from props | -| `Object.entries()` | 1 | `src/core/chart-api/chart-extra-navigation.ts` — iterating element attributes for `setAttribute` | -| `hasOwnProperty` | 0 | None found | - ---- - -## Proposed solution: type guard functions with `keyof` - -Replace each inline `'prop' in obj` check (Category 1, components repo only) with a **type guard function** that validates the property name at compile time: - -```ts -// BEFORE — 'operator' is an unchecked string, vulnerable to refactoring -if ('operator' in tokenOrGroup) { - // tokenOrGroup is narrowed to InternalToken -} - -// AFTER — 'operator' is validated against InternalToken via keyof -function isInternalToken( - t: InternalToken | InternalTokenGroup -): t is InternalToken { - const key: keyof InternalToken = 'operator'; // ← TypeScript validates this! - return key in t; -} - -if (isInternalToken(tokenOrGroup)) { - // tokenOrGroup is narrowed to InternalToken -} -``` - -**Why this works:** `keyof InternalToken` restricts the value to actual properties of `InternalToken`. If someone renames `operator` to `op`, the line `const key: keyof InternalToken = 'operator'` immediately produces a TypeScript error. - -### Type guards to create - -Each union type gets one or two type guard functions. All checks for that union are replaced with the same function call: - -| Type guard | Union | Discriminant | Replaces | -|-----------|-------|-------------|----------| -| `isInternalToken()` | `InternalToken \| InternalTokenGroup` | `operator` | 7 checks in property-filter | -| `isInternalTokenGroup()` | `InternalToken \| InternalTokenGroup` | `operation` | 2 checks in property-filter | -| `isOptionGroup()` | `Option \| OptionGroup` | `options` | 3 checks in property-filter, internal/option, select | -| `isAutosuggestGroup()` | `AutosuggestItem` union | `type` | 4 checks in autosuggest | -| `isAutosuggestOptionGroup()` | option vs group | `options` | 1 check in autosuggest | -| `isThresholdWithY()` | threshold series | `y` | 1 check in mixed-line-bar-chart | -| `isThresholdWithX()` | threshold series | `x` | 1 check in mixed-line-bar-chart | -| `isSideNavLink()` | SideNavigation item union | `href` | 1 check in side-navigation | -| `isSideNavSection()` | SideNavigation item union | `items` | 1 check in side-navigation | -| `isMenuDropdown()` | TopNavigation utility union | `items` | 1 check in top-navigation | -| `isItemGroup()` | ButtonDropdown item union | `items` | 1 check in button-dropdown | -| `isStackableItem()` | `StackableItem \| MessageDefinition` | `expandedIndex` | 2 checks in flashbar | -| `isReactRef()` | `HTMLElement \| React.RefObject` | `current` | 2 checks in flashbar | - -### Where type guards live - -Type guard functions will be co-located with the types they guard — typically in the same file as the interface definition or in a dedicated `utils.ts` within the component folder. For types shared across components (e.g., `OptionGroup` used in property-filter, internal/option, and select), the guard will live next to the shared type definition and be imported where needed. - -### What stays as-is - -The ~10 runtime/feature-detection checks in the components repo (Category 2) and all 12 checks in chart-components cannot use `keyof` because we don't own the types. These stay as inline `in` checks with an `eslint-disable` comment explaining why. - -Notable cases in components Category 2: - -- **`'keepOpenOnSelect' in option`** (property-filter): This property is intentionally absent from the public type definition — it's an internal runtime extension added programmatically to certain autosuggest options. Adding it to the type would expose it in the public API. Keeping the `in` check with an eslint-disable comment is the right approach. -- **`'id' in item`** (flashbar): `id` is an optional property on `FlashbarProps.MessageDefinition`. The `in` check tests for runtime key existence (whether the user provided the key at all), which is subtly different from `item.id !== undefined`. Both would work in practice, but we keep `in` here for semantic correctness. - -## Test coverage - -The COE action item asks to both refactor `in` checks and ensure test coverage. Our approach: - -1. Existing component tests that exercise union-type code paths (e.g., property filter with token groups, flashbar with stacked items) already provide integration-level coverage for the logic that depends on these checks. -2. We will verify that each `in` check code path is reachable from existing tests. Where coverage gaps are found, we will document them for follow-up but not block the refactoring on adding new integration tests. -3. The real testing opportunity is with the cases where a guard function **cannot** be introduced (e.g., `'keepOpenOnSelect' in option`, `'id' in item`). There, component-level tests that exercise both branches can fail conveniently if unsafe changes are made to the concerned code. - -## Future prevention: ESLint rule - -To prevent new raw `in` checks from being introduced, add an ESLint rule: - -```js -// eslint.config.mjs -{ - 'no-restricted-syntax': ['error', { - selector: 'BinaryExpression[operator="in"]', - message: 'Do not use the "in" operator directly. Use a typed type guard function instead.' - }] -} -``` - -The Category 2 usages get `// eslint-disable-next-line no-restricted-syntax` with an explanation. - -Note: This rule only catches the binary expression `'prop' in obj`, not `for...in` loops (which use `ForInStatement` in the AST, a different node type). - -### What this gives us - -| Scenario | Today | After this change | -|----------|-------|-------------------| -| Someone renames a property during refactoring | ❌ Silent failure at runtime (the COE bug) | ✅ TypeScript compile error in the type guard | -| Someone writes a new `'prop' in obj` check | ❌ No warning | ✅ ESLint error, must use a type guard | -| Browser feature detection | ✅ Works | ✅ Still works (explicit eslint-disable) | - -## Effort and risk - -**Estimated effort:** ~3-4 days of implementation + code review for the components repo. No code changes needed for chart-components. - -**Risk:** Low. Type guard functions are pure wrappers around the existing `in` logic — runtime behavior is identical. No public API, visual, or behavioral changes. If any issue is found, type guards can be inlined back to raw `in` checks with a simple find-and-replace. - -**Why now?** While no similar bug has recurred since the August 2024 COE, the risk is latent — any future refactoring of a discriminant property will silently break the corresponding `in` check. The type guard approach addresses this with minimal effort and also introduces an ESLint rule that prevents the pattern from proliferating further. Given that we found 40 usages across 20 files (and this number will only grow), addressing it now is cheaper than addressing it later. - -## Alternatives considered - -### Direct property access (`obj.prop`) - -Replace `'prop' in obj` with `obj.prop !== undefined`. - -- ✅ TypeScript validates property names -- ❌ Only works when the property exists on all types in the union (otherwise it's a type error) -- ❌ Doesn't narrow union types -- Only applicable to ~3 of the 40 cases - -### Discriminant tag property (`kind: 'token' | 'group'`) - -Add an explicit `kind` field to all union types. - -- ✅ Gold standard for discriminated unions -- ❌ Requires changing internal interfaces across many components -- ❌ Much larger scope (~2-3 weeks vs ~1 week) -- Could be done as a future improvement - -Adding a `kind` discriminant to e.g. `InternalToken | InternalTokenGroup` would require changing every call site that **creates** these objects (~15+ locations in property-filter alone), updating all test fixtures, and modifying serialization/deserialization logic. The type guard approach achieves the same compile-time safety with changes only at the ~30 **consumption** sites, not the creation sites. - -### Why not a `@typescript-eslint` rule? - -`@typescript-eslint` does not have a built-in rule that validates the left-hand side of `in` against the type of the right-hand side. Writing a custom type-aware ESLint rule is significantly more complex (requires the TypeScript type-checker integration) and not worth the effort when `no-restricted-syntax` + type guards already cover the problem completely. - -## Feedback - -Notes: - -* ... - -Voting: - -| Reviewer | Vote | Comments | -|----------|------|----------| -| | | | -| | | | -| | | | \ No newline at end of file