feat: chart improvements + accessibility + fixes

[alfredo1996]

Summary

Consolidated batch of 7 PRs (charts + accessibility + fixes):

Chart Improvements

• feat(component): add number formatting to single value and tooltips #169 Number formatting for single-value and tooltips
• feat(component): add DataZoom support for bar and line charts #170 DataZoom support for bar and line charts
• feat(component): auto-rotate and truncate axis labels #171 Auto-rotate and truncate axis labels
• feat(component): add reference lines (markLine) for bar and line charts #173 Reference lines (markLine) for bar and line charts
• feat(component): add donut center text and Top-N grouping for pie chart #174 Donut center text and Top-N grouping for pie chart

Accessibility and Fixes

• feat(component): accessibility — ARIA labels, keyboard nav, contrast fixes #178 ARIA labels, keyboard navigation, contrast improvements
• fix: bracket wrapping in code editor + radar chart scale #179 Fix bracket wrapping in code editor + radar chart scale

Test plan

• All 1086 component tests pass
• Build passes

Closes #138, #134, #137, #136, #139, #101, #103

Summary by CodeRabbit

• New Features

  • Reference lines (bar/line), data-zoom panning, axis-label rotation, decimal-places control, pie top-N grouping with "Other", donut center text, improved tooltip number formatting, and a Chart Catalog demo dashboard

• Improvements

  • Centralized chart formatting/utilities, consistent radar-axis scaling, deterministic graph fitting with loading overlay, richer chart click payloads (includes row data)

• Accessibility

  • ARIA chart descriptions and semantic, keyboard-friendly JSON viewer controls

• Tests

  • Expanded unit tests across chart utilities and chart components

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f1ebe3f8-8e3f-425d-9973-c2bdc940ea92

📥 Commits

Reviewing files that changed from the base of the PR and between 9be23f7 and beb9cbd.

📒 Files selected for processing (4)

• app/e2e/code-completion.spec.ts
• app/e2e/fixtures.ts
• app/src/components/chart-renderer.tsx
• scripts/seed-demo.mjs

---

Walkthrough

Adds chart utilities (formatting, axis-label heuristics, reference-line parsing, pie Top‑N), wires them into bar/line/pie/single-value/base charts, revises graph auto-fit and radar scaling, enriches chart click payloads, updates tests/mocks, and adjusts E2E CodeMirror dispatch/fallback behavior for temporary readonly initialization.

Changes

Cohort / File(s)	Summary
Chart utilities & new APIs component/src/charts/chart-utils.ts	New exported utilities/types: formatNumber, buildTooltipFormatter, buildCategoryAxisLabel, parseReferenceLines, buildMarkLineFromRefs, groupTopN, plus related option/type exports.
Chart components using utilities component/src/charts/bar-chart.tsx, component/src/charts/line-chart.tsx, component/src/charts/pie-chart.tsx, component/src/charts/single-value-chart.tsx	Bar/Line: added referenceLines?: string, parse/build markLine and attach to first series; Bar adds axisLabelRotation. Pie: added topN and donutCenterText, uses groupTopN and injects graphic for donut center. Single-value: now uses formatNumber, adds decimalPlaces prop and type alias change.
Base chart core, types & options registry component/src/charts/base-chart.tsx, component/src/charts/types.ts, component/src/components/composed/chart-options-schema.ts	Registered MarkLineComponent and GraphicComponent; added enableDataZoom and ariaDescription props/handling; conditional injection of dataZoom; chart options schema extended to expose new props.
Chart rendering & click payload app/src/components/chart-renderer.tsx	handleEChartsClick now merges the original clicked data row into the emitted payload; forwarded chart props extended to include new settings (axisLabelRotation, referenceLines, enableDataZoom, topN, donutCenterText, decimalPlaces).
Graph auto-fit & loading flow component/src/charts/graph-chart.tsx, component/src/charts/__tests__/graph-chart.test.tsx	Replaced RAF/timer auto-fit with layoutReady state and onLayoutDone wrapper; deterministic fitGraph() trigger; added loading overlay shown until layout ready; tests updated for overlay and layout lifecycle.
Radar scaling / chart-registry app/src/lib/chart-registry.ts, app/src/lib/__tests__/chart-registry.test.ts	Radar scaling changed to compute a single global max (long & wide formats); per-indicator explicit max accepted only when finite & >0; tests expanded for invalid/edge max values.
Tests & test setup component/vitest.setup.ts, component/src/charts/__tests__/*, component/src/components/composed/__tests__/*, app/e2e/fixtures.ts	Many new/updated unit tests for utilities and chart behaviors; ECharts mock extended with MarkLineComponent and GraphicComponent; CodeMirror E2E helper updated to dispatch CM6 insertion and the keyboard-fallback now triggers when dispatch result is "no-view" or "readonly".
Markdown, accessibility & UI small changes component/src/components/composed/markdown-widget.tsx, component/src/components/composed/json-viewer.tsx, component/src/components/composed/cross-filter-tag.tsx, component/src/components/composed/code-preview.tsx, component/src/components/composed/query-editor.tsx	Added GFM table parsing; JSON node toggle changed to semantic button with ARIA; remove-control rendering refactored to span role="button" when outer element is a button; removed opacity class from code-preview badge; added closeBrackets extension/keymap and corresponding test-mock updates.
Demo seed script scripts/seed-demo.mjs	Added buildChartCatalog(neo4jId) and buildChartImprovements(neo4jConnId) and extended seeding flow to create comprehensive chart demo dashboards and queries.

Sequence Diagram(s)

sequenceDiagram
  participant User as User
  participant CR as ChartRenderer
  participant CC as ChartComponent
  participant CU as chart-utils
  participant E as ECharts

  User->>E: click/chart interaction
  E->>CR: eCharts event payload
  CR->>CR: merge data row (if available) into payload
  CR->>CC: forward enriched click handler
  CC->>CR: (optionally) request markLine/formatter via CU
  CC->>CU: parseReferenceLines / buildTooltipFormatter
  CU-->>CC: markLine object / formatter
  CC->>E: setOption (includes markLine, tooltip.formatter)
  CR-->>User: onChartClick callback with enriched payload

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• release: v0.8 — UX & Charts #132 — Related CM6 E2E dispatch changes and editor typing helpers.
• feat(component): full chart configuration options (#20) #66 — Overlaps on chart-utility APIs and chart component behavior changes.
• fix(app): address CodeRabbit review findings from PR #82 #83 — Related modifications touching chart-renderer and click/prop forwarding.

Suggested labels

type: feature

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes in the PR: chart improvements (number formatting, DataZoom, axis labels, reference lines, pie grouping), accessibility enhancements (ARIA, keyboard nav), and fixes (bracket wrapping, radar scaling).
Docstring Coverage	✅ Passed	Docstring coverage is 82.76% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch release/chart-improvements

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (3)

app/e2e/fixtures.ts (1)

151-164: Keyboard fallback for "readonly" may silently fail to insert text.

When CM6's internal readOnly state is true, keyboard input can be silently dropped (as noted in the docstring on lines 56-59). The fallback attempts Ctrl+A → Backspace → insertText, then verifies insertion on line 160.

Two concerns:

1. Clearing may also be dropped: If the editor is readonly, the Backspace might not clear existing content. If old content happens to contain the first 20 chars of the new query, the check passes incorrectly (edge case but possible).

2. Masking stability issues: If "readonly" is returned after the 600ms stability window passes, it suggests the pre-dispatch checks (lines 82-106) aren't sufficient. Falling back to keyboard rather than improving the stability detection could lead to flaky tests.

Consider either:

• Adding await page.keyboard.press("Backspace") verification (check content is empty before insert), or
• Logging/tracking when "readonly" fallback is taken to catch recurring issues.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/e2e/fixtures.ts` around lines 151 - 164, The readonly keyboard-fallback
can silently fail because Backspace may be ignored; update the block handling
dispatched === "readonly" in fixtures.ts to explicitly verify the clear step
before inserting: after page.keyboard.press("Backspace") check cm.textContent()
(or await expect(cm).toHaveText("")/empty) and if it’s not empty throw or retry
once, then proceed to page.keyboard.insertText(query); additionally emit a
test-side log/metric when the readonly fallback path (dispatched === "readonly")
is taken so we can detect recurring flaky usage of this fallback (reference
symbols: dispatched, cm, page.keyboard.press, query).

component/src/charts/__tests__/base-chart.test.tsx (1)

30-40: Centralize the echarts/components mock.

component/vitest.setup.ts already owns this mock shape. Keeping a second copy here is what let MarkLineComponent and GraphicComponent drift out of sync. Prefer one shared mock source.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/__tests__/base-chart.test.tsx` around lines 30 - 40,
Remove the duplicate vi.mock("echarts/components", ...) block from the test and
let the centralized mock in vitest.setup.ts provide the component stubs; if
tests fail after removal, update the central mock in vitest.setup.ts to include
any missing symbols (e.g., MarkLineComponent, GraphicComponent) so the shared
mock shape matches what tests expect, and avoid redefining the mock in
base-chart.test.tsx.

component/src/charts/chart-utils.ts (1)

307-307: Move import to top of file.

Imports should be grouped at the top for consistency and readability.

Suggested fix

 import type { EChartsOption } from "echarts";
 import type { ColorThreshold } from "./color-threshold";
 import { resolveThresholdColor } from "./color-threshold";
 import type { StylingRule } from "./styling-rule";
 import { resolveStylingRuleColor } from "./styling-rule";
+import type { PieChartDataPoint } from "./types";

Then remove line 307.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/chart-utils.ts` at line 307, The import "import type {
PieChartDataPoint } from \"./types\";" is placed mid-file; move this import
statement up into the existing import group at the top of chart-utils.ts so all
imports are grouped together, then remove the duplicate import line currently at
line 307; ensure any references to the PieChartDataPoint type in functions
(e.g., functions that use PieChartDataPoint within this file) continue to
compile after relocation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@component/src/charts/__tests__/pie-utils.test.ts`:
- Around line 5-44: The test-suite misses an unsorted input case and groupTopN
assumes input is pre-sorted; add a test that passes an unsorted data array to
groupTopN to catch this, and fix groupTopN to select the top N by value
regardless of input order (e.g., inside groupTopN make a shallow copy and sort
by value DESC to pick topN, then aggregate the rest into "Other"); reference
groupTopN in tests and pie-chart.tsx's sortSlices flag to ensure behavior is
consistent when sortSlices=false.

In `@component/src/charts/bar-chart.tsx`:
- Around line 92-109: The mark line builder currently assumes reference values
go on the yAxis; update the buildMarkLineFromRefs function to accept an axis
parameter ('xAxis' | 'yAxis') defaulting to 'yAxis' and use that key for each
entry, then update callers (e.g., bar-chart.tsx where markLine is created from
parseReferenceLines(referenceLinesJson)) to pass axis: use "xAxis" when
orientation === "horizontal" and "yAxis" otherwise; also add a unit test for
orientation="horizontal" to assert the markLine places values on xAxis to
prevent regressions.

In `@component/src/charts/base-chart.tsx`:
- Around line 155-165: The current merge in merged (in BaseChartProps rendering)
unconditionally overwrites caller-supplied options.dataZoom when enableDataZoom
is true; change it to preserve and merge with any existing options.dataZoom: if
options.dataZoom is undefined, set the default inside x/y entries as now,
otherwise merge/concat only missing inside sliders (or leave caller array
intact), e.g., compute a finalDataZoom from options.dataZoom and the default
[{type:"inside", xAxisIndex:0},{type:"inside", yAxisIndex:0}] and assign
dataZoom: finalDataZoom in the merged object so enableDataZoom augments rather
than replaces user-provided dataZoom.

In `@component/src/charts/pie-chart.tsx`:
- Around line 29-30: The component currently uses the nullish coalescing
operator when deciding the donut center label, so an explicit empty string
(donutCenterText === "") is treated as a provided value and renders blank;
update the logic where the center text is chosen (references: prop
donutCenterText in pie-chart.tsx and the render/compute function that currently
uses "??") to treat empty string as "use auto-total" (e.g., choose
donutCenterText if it's non-empty, otherwise compute and use the total). Apply
the same change to the other occurrences mentioned so that "" falls back to the
computed total rather than rendering blank.

In `@component/src/charts/single-value-chart.tsx`:
- Around line 37-38: The decimalPlaces prop is documented as 0–6 but currently
only checked with >= 0, so values >6 (or <0 besides -1) leak through; before
formatting (where you currently test decimalPlaces >= 0) clamp/normalize the
prop to the allowed range by: treat undefined or -1 as automatic mode, otherwise
constrain numeric values to Math.max(0, Math.min(6, decimalPlaces)) (or a clamp
helper) and use that clamped value in the formatting logic; update both the prop
handling site (decimalPlaces?) and the formatter call (the location that
currently checks ">= 0") so out-of-range inputs fall back to automatic or the
clamped precision.

In `@component/src/components/composed/chart-options-schema.ts`:
- Around line 82-86: Bar/line charts don't expose the shared number-formatting
options so buildTooltipFormatter() is always called with defaults; update the
composed schema and components to add those missing formatter keys. Add the keys
numberFormat, decimalPlaces, prefix, and suffix to the barOptions and
lineOptions arrays in chart-options-schema.ts (mirror how singleValueOptions
defines them), and then add corresponding props/propTypes/defaults to the
BarChart and LineChart components so they pass these props into their tooltip
config where buildTooltipFormatter(...) is invoked. Ensure the prop names match
the schema keys and are forwarded into the tooltip formatter call (e.g.,
buildTooltipFormatter({ numberFormat, decimalPlaces, prefix, suffix })) so
tooltips honor configured formatting.

---

Nitpick comments:
In `@app/e2e/fixtures.ts`:
- Around line 151-164: The readonly keyboard-fallback can silently fail because
Backspace may be ignored; update the block handling dispatched === "readonly" in
fixtures.ts to explicitly verify the clear step before inserting: after
page.keyboard.press("Backspace") check cm.textContent() (or await
expect(cm).toHaveText("")/empty) and if it’s not empty throw or retry once, then
proceed to page.keyboard.insertText(query); additionally emit a test-side
log/metric when the readonly fallback path (dispatched === "readonly") is taken
so we can detect recurring flaky usage of this fallback (reference symbols:
dispatched, cm, page.keyboard.press, query).

In `@component/src/charts/__tests__/base-chart.test.tsx`:
- Around line 30-40: Remove the duplicate vi.mock("echarts/components", ...)
block from the test and let the centralized mock in vitest.setup.ts provide the
component stubs; if tests fail after removal, update the central mock in
vitest.setup.ts to include any missing symbols (e.g., MarkLineComponent,
GraphicComponent) so the shared mock shape matches what tests expect, and avoid
redefining the mock in base-chart.test.tsx.

In `@component/src/charts/chart-utils.ts`:
- Line 307: The import "import type { PieChartDataPoint } from \"./types\";" is
placed mid-file; move this import statement up into the existing import group at
the top of chart-utils.ts so all imports are grouped together, then remove the
duplicate import line currently at line 307; ensure any references to the
PieChartDataPoint type in functions (e.g., functions that use PieChartDataPoint
within this file) continue to compile after relocation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dd633b35-4d62-4806-95b6-ce33e98c49ea

📥 Commits

Reviewing files that changed from the base of the PR and between 59e78c2 and e3448e6.

📒 Files selected for processing (18)

• app/e2e/fixtures.ts
• component/src/charts/__tests__/axis-label-utils.test.ts
• component/src/charts/__tests__/base-chart.test.tsx
• component/src/charts/__tests__/format-number.test.ts
• component/src/charts/__tests__/line-chart.test.tsx
• component/src/charts/__tests__/pie-utils.test.ts
• component/src/charts/__tests__/reference-line.test.ts
• component/src/charts/__tests__/single-value-chart.test.tsx
• component/src/charts/__tests__/tooltip-formatter.test.ts
• component/src/charts/bar-chart.tsx
• component/src/charts/base-chart.tsx
• component/src/charts/chart-utils.ts
• component/src/charts/line-chart.tsx
• component/src/charts/pie-chart.tsx
• component/src/charts/single-value-chart.tsx
• component/src/charts/types.ts
• component/src/components/composed/chart-options-schema.ts
• component/vitest.setup.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add an unsorted topN case.

groupTopN() assumes descending input, but component/src/charts/pie-chart.tsx:75-78 passes raw data when sortSlices is false. With the default sortSlices=false, topN keeps the first N rows, not the largest N slices. This suite only uses pre-sorted fixtures, so it will not catch the bug.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/__tests__/pie-utils.test.ts` around lines 5 - 44, The
test-suite misses an unsorted input case and groupTopN assumes input is
pre-sorted; add a test that passes an unsorted data array to groupTopN to catch
this, and fix groupTopN to select the top N by value regardless of input order
(e.g., inside groupTopN make a shallow copy and sort by value DESC to pick topN,
then aggregate the rest into "Other"); reference groupTopN in tests and
pie-chart.tsx's sortSlices flag to ensure behavior is consistent when
sortSlices=false.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== buildMarkLineFromRefs ==="
rg -n -A30 -B2 'export function buildMarkLineFromRefs\(' component/src/charts/chart-utils.ts

echo
echo "=== BarChart orientation + markLine wiring ==="
sed -n '88,146p' component/src/charts/bar-chart.tsx

echo
echo "=== Existing reference-line tests ==="
sed -n '1,220p' component/src/charts/__tests__/reference-line.test.ts

Repository: alfredo1996/neoboard

Length of output: 7449

---

Pass axis parameter to handle horizontal bar orientation.

buildMarkLineFromRefs() hardcodes yAxis for reference line coordinates, but when orientation === "horizontal" the yAxis becomes the category axis. Reference lines must target the value axis instead: use xAxis for horizontal bars and yAxis for vertical.

Suggested fix

-    const markLine = buildMarkLineFromRefs(refLines);
+    const markLine = buildMarkLineFromRefs(refLines, isHorizontal ? "xAxis" : "yAxis");

// component/src/charts/chart-utils.ts
export function buildMarkLineFromRefs(
  lines: ReferenceLine[],
  axis: "xAxis" | "yAxis" = "yAxis",
) {
  if (!lines.length) return undefined;
  return {
    silent: true,
    symbol: "none",
    data: lines.map((line) => ({
      [axis]: line.value,
      label: {
        formatter: line.label ?? String(line.value),
        position: "insideEndTop" as const,
      },
      lineStyle: {
        color: line.color ?? "#888",
        type: "dashed" as const,
      },
    })),
  };
}

Add a test case for orientation="horizontal" to prevent regression.

Also applies to: 123–145

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/bar-chart.tsx` around lines 92 - 109, The mark line
builder currently assumes reference values go on the yAxis; update the
buildMarkLineFromRefs function to accept an axis parameter ('xAxis' | 'yAxis')
defaulting to 'yAxis' and use that key for each entry, then update callers
(e.g., bar-chart.tsx where markLine is created from
parseReferenceLines(referenceLinesJson)) to pass axis: use "xAxis" when
orientation === "horizontal" and "yAxis" otherwise; also add a unit test for
orientation="horizontal" to assert the markLine places values on xAxis to
prevent regressions.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t overwrite caller-supplied dataZoom.

BaseChartProps.options is documented as raw ECharts config, but this branch replaces any options.dataZoom whenever enableDataZoom is true. That breaks advanced callers that already provide custom slider/inside zoom config.

Possible fix

-      ...(enableDataZoom
+      ...(enableDataZoom && options?.dataZoom == null
         ? {
             dataZoom: [
               { type: "inside", xAxisIndex: 0 },
               { type: "inside", yAxisIndex: 0 },
             ],
           }
         : {}),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/base-chart.tsx` around lines 155 - 165, The current
merge in merged (in BaseChartProps rendering) unconditionally overwrites
caller-supplied options.dataZoom when enableDataZoom is true; change it to
preserve and merge with any existing options.dataZoom: if options.dataZoom is
undefined, set the default inside x/y entries as now, otherwise merge/concat
only missing inside sliders (or leave caller array intact), e.g., compute a
finalDataZoom from options.dataZoom and the default [{type:"inside",
xAxisIndex:0},{type:"inside", yAxisIndex:0}] and assign dataZoom: finalDataZoom
in the merged object so enableDataZoom augments rather than replaces
user-provided dataZoom.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Honor the “empty = auto-total” contract for donutCenterText.

Line 130 uses ??, so donutCenterText="" renders a blank center label instead of falling back to the computed total.

🔧 Possible fix

-            text: donutCenterText ?? String(sortedData.reduce((s, d) => s + d.value, 0)),
+            text: donutCenterText && donutCenterText.trim().length > 0
+              ? donutCenterText
+              : String(sortedData.reduce((s, d) => s + d.value, 0)),

Also applies to: 57-57, 123-136

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/pie-chart.tsx` around lines 29 - 30, The component
currently uses the nullish coalescing operator when deciding the donut center
label, so an explicit empty string (donutCenterText === "") is treated as a
provided value and renders blank; update the logic where the center text is
chosen (references: prop donutCenterText in pie-chart.tsx and the render/compute
function that currently uses "??") to treat empty string as "use auto-total"
(e.g., choose donutCenterText if it's non-empty, otherwise compute and use the
total). Apply the same change to the other occurrences mentioned so that ""
falls back to the computed total rather than rendering blank.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Clamp decimalPlaces to the documented 0–6 range before formatting.

The prop contract says 0–6, but Line 91 only checks >= 0. Out-of-range values currently leak through the public API instead of falling back to automatic mode.

🔧 Possible fix

-      const dp = decimalPlaces !== undefined && decimalPlaces >= 0 ? decimalPlaces : undefined;
+      const dp =
+        Number.isInteger(decimalPlaces) && decimalPlaces !== undefined && decimalPlaces >= 0
+          ? Math.min(decimalPlaces, 6)
+          : undefined;

Also applies to: 66-66, 91-92

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/single-value-chart.tsx` around lines 37 - 38, The
decimalPlaces prop is documented as 0–6 but currently only checked with >= 0, so
values >6 (or <0 besides -1) leak through; before formatting (where you
currently test decimalPlaces >= 0) clamp/normalize the prop to the allowed range
by: treat undefined or -1 as automatic mode, otherwise constrain numeric values
to Math.max(0, Math.min(6, decimalPlaces)) (or a clamp helper) and use that
clamped value in the formatting logic; update both the prop handling site
(decimalPlaces?) and the formatter call (the location that currently checks ">=
0") so out-of-range inputs fall back to automatic or the clamped precision.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Formatter-related props used in chart components ==="
rg -n --type=ts --type=tsx '\b(numberFormat|decimalPlaces|prefix|suffix)\b' component/src/charts

echo
echo "=== Formatter-related keys exposed by chart-options-schema.ts ==="
rg -n --type=ts '\b(numberFormat|decimalPlaces|prefix|suffix)\b' component/src/components/composed/chart-options-schema.ts

echo
echo "=== How chart-options-panel consumes schema keys ==="
rg -n --type=tsx 'getChartOptions|opt\.key' component/src/components/composed/chart-options-panel.tsx

Repository: alfredo1996/neoboard

Length of output: 165

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Formatter-related props used in chart components ==="
find component/src/charts -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs rg -n '\b(numberFormat|decimalPlaces|prefix|suffix)\b' 2>/dev/null || echo "(no matches or error)"

echo
echo "=== Formatter-related keys in chart-options-schema.ts ==="
rg -n '\b(numberFormat|decimalPlaces|prefix|suffix)\b' component/src/components/composed/chart-options-schema.ts 2>/dev/null || echo "(no matches)"

echo
echo "=== Relevant sections from chart-options-schema.ts ==="
cat -n component/src/components/composed/chart-options-schema.ts | head -100

echo
echo "=== Chart types and their option definitions ==="
rg -n 'const.*Options.*ChartOptionDef\[\]|case.*:' component/src/components/composed/chart-options-schema.ts | head -30

Repository: alfredo1996/neoboard

Length of output: 18382

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Bar Chart Props Interface ==="
rg -A 20 'interface.*BarChartProps|type.*BarChartProps' component/src/charts/ | head -40

echo
echo "=== Line Chart Props Interface ==="
rg -A 20 'interface.*LineChartProps|type.*LineChartProps' component/src/charts/ | head -40

echo
echo "=== Pie Chart Props Interface ==="
rg -A 20 'interface.*PieChartProps|type.*PieChartProps' component/src/charts/ | head -40

echo
echo "=== Check if bar/line/pie use tooltip formatters ==="
rg -n 'buildTooltipFormatter|tooltip.*format|formatNumber' component/src/charts/bar-chart.tsx component/src/charts/line-chart.tsx component/src/charts/pie-chart.tsx 2>/dev/null || echo "(no matches)"

Repository: alfredo1996/neoboard

Length of output: 9303

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Bar chart tooltip formatter call ==="
rg -B 2 -A 2 'buildTooltipFormatter\(\)' component/src/charts/bar-chart.tsx

echo
echo "=== Line chart tooltip formatter call ==="
rg -B 2 -A 2 'buildTooltipFormatter\(\)' component/src/charts/line-chart.tsx

echo
echo "=== Pie chart tooltip formatter call ==="
rg -B 2 -A 2 'buildTooltipFormatter\(\)' component/src/charts/pie-chart.tsx 2>/dev/null || echo "(no tooltip formatter found)"

echo
echo "=== buildTooltipFormatter signature ==="
rg -B 2 -A 8 'export.*function buildTooltipFormatter|export const buildTooltipFormatter' component/src/charts/chart-utils.ts

Repository: alfredo1996/neoboard

Length of output: 1254

---

Formatter options missing from bar/line chart schema—blocks shared number formatting feature.

Bar and line charts call buildTooltipFormatter() in their tooltip configuration, but with no arguments (defaults only). Neither component exposes numberFormat, decimalPlaces, prefix, or suffix props, and the schema doesn't expose these keys for bar/line either. This prevents users from configuring number formatting in tooltips via the composed options panel, leaving the PR objective of "shared number formatting across all charts" incomplete. Add formatter options to barOptions and lineOptions schema arrays and corresponding props to the chart components.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/components/composed/chart-options-schema.ts` around lines 82 -
86, Bar/line charts don't expose the shared number-formatting options so
buildTooltipFormatter() is always called with defaults; update the composed
schema and components to add those missing formatter keys. Add the keys
numberFormat, decimalPlaces, prefix, and suffix to the barOptions and
lineOptions arrays in chart-options-schema.ts (mirror how singleValueOptions
defines them), and then add corresponding props/propTypes/defaults to the
BarChart and LineChart components so they pass these props into their tooltip
config where buildTooltipFormatter(...) is invoked. Ensure the prop names match
the schema keys and are forwarded into the tooltip formatter call (e.g.,
buildTooltipFormatter({ numberFormat, decimalPlaces, prefix, suffix })) so
tooltips honor configured formatting.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

scripts/seed-demo.mjs (1)

2402-2403: Page comment numbering is off by one.

This says "Page 13" but follows the Graph Chart page which is already "Page 13" (line 2380). Should be "Page 14". Subsequent pages have the same off-by-one error.

Fix page numbering

-      // ── Page 13: Parameter Widgets ─────────────────────────────────
+      // ── Page 14: Parameter Widgets ─────────────────────────────────

And similarly for subsequent pages (Form Widget → 15, Behavior Options → 16, etc.).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/seed-demo.mjs` around lines 2402 - 2403, Update the off-by-one page
numbering in the seed demo headings: change the comment "// ── Page 13:
Parameter Widgets ─────────────────────────────────" to "// ── Page 14:
Parameter Widgets ─────────────────────────────────" and increment the page
numbers for all subsequent page header comments (e.g., "Form Widget", "Behavior
Options", etc.) so they follow the correct sequence (Form Widget → 15, Behavior
Options → 16, etc.); locate these headers in scripts/seed-demo.mjs by searching
for the page header comment pattern "// ── Page" and adjust each numeric label
accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/src/lib/chart-registry.ts`:
- Around line 496-500: The current logic that sets indicatorExplicitMax in the
block inside chart-registry.ts only keeps the first valid explicitMax per
indName, causing later larger maxima to be ignored; change the behavior in the
code that reads maxKey/explicitMax (the block that calls Number(r[maxKey]) and
indicatorExplicitMax.set(indName, explicitMax)) to compute and store the highest
valid positive finite explicitMax per indName (e.g., if an entry already exists
use Math.max(existing, explicitMax)); preserve the validation (Number.isFinite
&& > 0) and do not overwrite with invalid values, and add a regression test that
feeds two rows with the same indicator and different valid max values to assert
the stored explicit max is the larger one (or alternately fail on disagreement
if that policy is preferred).

In `@component/src/components/composed/__tests__/markdown-widget.test.tsx`:
- Around line 439-441: The test in markdown-widget.test.tsx uses
container.innerHTML.indexOf("</ul>") and indexOf("<table") then compares
ulCloseIndex < tableIndex, which can be a false positive if either index is -1;
update the test (in the same test block that defines ulCloseIndex and
tableIndex) to first assert both markers exist (e.g.,
expect(ulCloseIndex).toBeGreaterThanOrEqual(0) and
expect(tableIndex).toBeGreaterThanOrEqual(0) or similar) and only then assert
the ordering (expect(ulCloseIndex).toBeLessThan(tableIndex)), so that missing
markers fail explicitly instead of passing the order check.

In `@component/src/components/composed/cross-filter-tag.tsx`:
- Around line 31-56: The span used for removeControl when onClick is present
lacks visible focus styling for keyboard users; update the span (the element
created in the onClick ? branch of removeControl) to include accessible focus
styles (e.g., add focus/focus-visible classes or an outline/ring style) so it
shows a clear focus indicator when keyboard-focused, while preserving the
existing role="button", tabIndex, onClick and onKeyDown behavior; ensure the
styling matches the project design tokens (e.g., ring or outline classes) and
does not remove the sr-only label or existing stopPropagation logic.

In `@scripts/seed-demo.mjs`:
- Around line 2027-2034: The clickNavPage helper is defined but never used;
remove the dead closure or wire it to a widget so a user can navigate to
detailPageId. Either (A) delete the clickNavPage function and any unused
imports, or (B) attach a "navigate-to-page" rule to a widget (e.g., the Bar
Chart page/widget) by creating a rule using clickNavPage(triggerColumn,
detailPageId) (type "navigate-to-page" with triggerColumn and targetPageId set
to detailPageId) so the widget triggers navigation to the Detail View; ensure
you update the widget's rules array where rules are defined.

---

Nitpick comments:
In `@scripts/seed-demo.mjs`:
- Around line 2402-2403: Update the off-by-one page numbering in the seed demo
headings: change the comment "// ── Page 13: Parameter Widgets
─────────────────────────────────" to "// ── Page 14: Parameter Widgets
─────────────────────────────────" and increment the page numbers for all
subsequent page header comments (e.g., "Form Widget", "Behavior Options", etc.)
so they follow the correct sequence (Form Widget → 15, Behavior Options → 16,
etc.); locate these headers in scripts/seed-demo.mjs by searching for the page
header comment pattern "// ── Page" and adjust each numeric label accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cadc1765-5f21-4e47-b8c5-383bb68df292

📥 Commits

Reviewing files that changed from the base of the PR and between e3448e6 and 0abfc99.

⛔ Files ignored due to path filters (2)

• app/package-lock.json is excluded by !**/package-lock.json
• docker/neo4j/init.cypher is excluded by !docker/**

📒 Files selected for processing (16)

• app/src/lib/__tests__/chart-registry.test.ts
• app/src/lib/chart-registry.ts
• component/src/charts/__tests__/graph-chart.test.tsx
• component/src/charts/base-chart.tsx
• component/src/charts/chart-utils.ts
• component/src/charts/graph-chart.tsx
• component/src/charts/types.ts
• component/src/components/composed/__tests__/markdown-widget.test.tsx
• component/src/components/composed/__tests__/query-editor.test.tsx
• component/src/components/composed/chart-options-schema.ts
• component/src/components/composed/code-preview.tsx
• component/src/components/composed/cross-filter-tag.tsx
• component/src/components/composed/json-viewer.tsx
• component/src/components/composed/markdown-widget.tsx
• component/src/components/composed/query-editor.tsx
• scripts/seed-demo.mjs

✅ Files skipped from review due to trivial changes (4)

• component/src/components/composed/tests/query-editor.test.tsx
• component/src/charts/types.ts
• component/src/components/composed/code-preview.tsx
• component/src/components/composed/markdown-widget.tsx

🚧 Files skipped from review as they are similar to previous changes (3)

• component/src/components/composed/chart-options-schema.ts
• component/src/charts/base-chart.tsx
• component/src/charts/chart-utils.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use the highest valid max per indicator, not the first one.

Line 498 makes the chosen explicit max depend on row order. In long-format radar data the same indicator is repeated across rows, so a later row can carry a larger valid max. Keeping the first one can mis-scale that axis for the later series. Please aggregate valid maxima per indicator (or fail when they disagree), and add a regression test for two rows with the same indicator but different valid max values.

Suggested fix

       if (maxKey) {
         const explicitMax = Number(r[maxKey]);
-        if (Number.isFinite(explicitMax) && explicitMax > 0 && !indicatorExplicitMax.has(indName)) {
-          indicatorExplicitMax.set(indName, explicitMax);
+        if (Number.isFinite(explicitMax) && explicitMax > 0) {
+          indicatorExplicitMax.set(
+            indName,
+            Math.max(indicatorExplicitMax.get(indName) ?? 0, explicitMax),
+          );
         }
       }

📝 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

	if (maxKey) {
	const explicitMax = Number(r[maxKey]) || 100;
	if (!indicatorExplicitMax.has(indName)) indicatorExplicitMax.set(indName, explicitMax);
	const explicitMax = Number(r[maxKey]);
	if (Number.isFinite(explicitMax) && explicitMax > 0 && !indicatorExplicitMax.has(indName)) {
	indicatorExplicitMax.set(indName, explicitMax);
	}
	if (maxKey) {
	const explicitMax = Number(r[maxKey]);
	if (Number.isFinite(explicitMax) && explicitMax > 0) {
	indicatorExplicitMax.set(
	indName,
	Math.max(indicatorExplicitMax.get(indName) ?? 0, explicitMax),
	);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@app/src/lib/chart-registry.ts` around lines 496 - 500, The current logic that
sets indicatorExplicitMax in the block inside chart-registry.ts only keeps the
first valid explicitMax per indName, causing later larger maxima to be ignored;
change the behavior in the code that reads maxKey/explicitMax (the block that
calls Number(r[maxKey]) and indicatorExplicitMax.set(indName, explicitMax)) to
compute and store the highest valid positive finite explicitMax per indName
(e.g., if an entry already exists use Math.max(existing, explicitMax)); preserve
the validation (Number.isFinite && > 0) and do not overwrite with invalid
values, and add a regression test that feeds two rows with the same indicator
and different valid max values to assert the stored explicit max is the larger
one (or alternately fail on disagreement if that policy is preferred).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Guard against false-positive ordering assertion.

At Line 439, indexOf("</ul>") can be -1; then -1 < tableIndex still passes even when list-closing is missing. Assert both markers exist before comparing order.

Suggested fix

   const ulCloseIndex = container.innerHTML.indexOf("</ul>");
   const tableIndex = container.innerHTML.indexOf("<table");
+  expect(ulCloseIndex).toBeGreaterThanOrEqual(0);
+  expect(tableIndex).toBeGreaterThanOrEqual(0);
   expect(ulCloseIndex).toBeLessThan(tableIndex);

📝 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

	const ulCloseIndex = container.innerHTML.indexOf("</ul>");
	const tableIndex = container.innerHTML.indexOf("<table");
	expect(ulCloseIndex).toBeLessThan(tableIndex);
	const ulCloseIndex = container.innerHTML.indexOf("</ul>");
	const tableIndex = container.innerHTML.indexOf("<table");
	expect(ulCloseIndex).toBeGreaterThanOrEqual(0);
	expect(tableIndex).toBeGreaterThanOrEqual(0);
	expect(ulCloseIndex).toBeLessThan(tableIndex);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/components/composed/__tests__/markdown-widget.test.tsx` around
lines 439 - 441, The test in markdown-widget.test.tsx uses
container.innerHTML.indexOf("</ul>") and indexOf("<table") then compares
ulCloseIndex < tableIndex, which can be a false positive if either index is -1;
update the test (in the same test block that defines ulCloseIndex and
tableIndex) to first assert both markers exist (e.g.,
expect(ulCloseIndex).toBeGreaterThanOrEqual(0) and
expect(tableIndex).toBeGreaterThanOrEqual(0) or similar) and only then assert
the ordering (expect(ulCloseIndex).toBeLessThan(tableIndex)), so that missing
markers fail explicitly instead of passing the order check.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Nested-button fix is correct; add visible focus styling for keyboard users.

The <span role="button"> approach is the right solution for avoiding invalid nested <button> elements. However, line 41 is missing focus styling — unlike native <button> elements, <span> has no browser-default focus ring, so keyboard users won't see a visible indicator.

🛡️ Proposed fix to add focus styling

        <span
          role="button"
          tabIndex={0}
          onClick={(e) => { e.stopPropagation(); onRemove(); }}
          onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.stopPropagation(); e.preventDefault(); onRemove(); } }}
-         className="ml-1 rounded-full p-0.5 hover:bg-muted cursor-pointer"
+         className="ml-1 rounded-full p-0.5 hover:bg-muted cursor-pointer focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-1"
        >

📝 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

	// When onClick is set the outer element is a <button>, so the remove
	// control must NOT be a <button> (nested buttons are invalid HTML and
	// cause React hydration errors). Use a <span role="button"> instead.
	const removeControl = onRemove && (
	onClick ? (
	<span
	role="button"
	tabIndex={0}
	onClick={(e) => { e.stopPropagation(); onRemove(); }}
	onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.stopPropagation(); e.preventDefault(); onRemove(); } }}
	className="ml-1 rounded-full p-0.5 hover:bg-muted cursor-pointer"
	>
	<X className="h-3 w-3" />
	<span className="sr-only">Remove cross-filter</span>
	</span>
	) : (
	<button
	type="button"
	onClick={(e) => { e.stopPropagation(); onRemove(); }}
	className="ml-1 rounded-full p-0.5 hover:bg-muted"
	>
	<X className="h-3 w-3" />
	<span className="sr-only">Remove cross-filter</span>
	</button>
	)
	);
	// When onClick is set the outer element is a <button>, so the remove
	// control must NOT be a <button> (nested buttons are invalid HTML and
	// cause React hydration errors). Use a <span role="button"> instead.
	const removeControl = onRemove && (
	onClick ? (
	<span
	role="button"
	tabIndex={0}
	onClick={(e) => { e.stopPropagation(); onRemove(); }}
	onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.stopPropagation(); e.preventDefault(); onRemove(); } }}
	className="ml-1 rounded-full p-0.5 hover:bg-muted cursor-pointer focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-1"
	>
	<X className="h-3 w-3" />
	<span className="sr-only">Remove cross-filter</span>
	</span>
	) : (
	<button
	type="button"
	onClick={(e) => { e.stopPropagation(); onRemove(); }}
	className="ml-1 rounded-full p-0.5 hover:bg-muted"
	>
	<X className="h-3 w-3" />
	<span className="sr-only">Remove cross-filter</span>
	</button>
	)
	);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@component/src/components/composed/cross-filter-tag.tsx` around lines 31 - 56,
The span used for removeControl when onClick is present lacks visible focus
styling for keyboard users; update the span (the element created in the onClick
? branch of removeControl) to include accessible focus styles (e.g., add
focus/focus-visible classes or an outline/ring style) so it shows a clear focus
indicator when keyboard-focused, while preserving the existing role="button",
tabIndex, onClick and onKeyDown behavior; ensure the styling matches the project
design tokens (e.g., ring or outline classes) and does not remove the sr-only
label or existing stopPropagation logic.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

clickNavPage helper is defined but never used.

This closure is dead code. Either wire it up to navigate to detailPageId from a widget, or remove it.

Note: The Detail View page (line 2580) uses detailPageId but no widget actually navigates there.

Suggested fix: wire navigation or remove dead code

Option A — Remove unused helper:

-  // Click action: navigate to page
-  const clickNavPage = (triggerCol, pageId) => ({
-    type: "navigate-to-page",
-    rules: [{
-      id: uuid(), type: "navigate-to-page",
-      triggerColumn: triggerCol,
-      targetPageId: pageId,
-    }],
-  });

Option B — Wire a widget to navigate to the detail page (e.g., on the Bar Chart page):

          // Bar with click action
          { id: uuid(), chartType: "bar", connectionId: neo4jId, query: Q.barData,
-           settings: { title: "Click → Set Parameter", clickAction: clickSetParam("label", "bar_decade") } },
+           settings: { title: "Click → Navigate to Detail", clickAction: clickNavPage("label", detailPageId) } },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/seed-demo.mjs` around lines 2027 - 2034, The clickNavPage helper is
defined but never used; remove the dead closure or wire it to a widget so a user
can navigate to detailPageId. Either (A) delete the clickNavPage function and
any unused imports, or (B) attach a "navigate-to-page" rule to a widget (e.g.,
the Bar Chart page/widget) by creating a rule using clickNavPage(triggerColumn,
detailPageId) (type "navigate-to-page" with triggerColumn and targetPageId set
to detailPageId) so the widget triggers navigation to the Detail View; ensure
you update the widget's rules array where rules are defined.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud