feat: accessibility + editor/radar fixes - #186
Closed
alfredo1996 wants to merge 16 commits into
Closed
Conversation
…trast #103 — ECharts ARIA: - Add ariaDescription prop to BaseChart for custom screen reader descriptions - Wire aria.label.description into merged ECharts options - Add role="img" and tabIndex={0} to chart container for keyboard focus #101 — Accessibility audit fixes: - CodePreview: fix contrast by removing /50 opacity on language label - JsonViewer: replace div with button for expand/collapse, add aria-expanded #102 — Keyboard navigation: - Chart containers now focusable via tabIndex={0} - JsonViewer nodes navigable via keyboard (native button focus) Closes #103, Closes #101, Closes #102 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bug 1 — CodeMirror bracket wrapping: - Add closeBrackets() extension and closeBracketsKeymap to query editor - Selecting text and typing ( now wraps as (text) instead of replacing - Both Cypher and SQL bracket facets are now activated Bug 2 — Radar chart uniform shape: - Use single global max across all indicators instead of per-indicator max - Values like 172 vs 9 now show actual relative differences - Explicit max column values preserved when provided - 4 new/updated tests for global max behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New "Chart Catalog" dashboard with 12 pages, one per chart type: - Bar: vertical, horizontal, stacked, show values, styling, click, colorblind + 6 palettes - Line: default, smooth+area, stepped, show points, colorblind + 6 palettes - Pie: default, donut, rose, labels inside, click, colorblind + 6 palettes - Single Value: prefix/suffix, comma, compact, styling, trend - Table: default, sorting+filters, selection, click - Gauge: default, no pointer, half gauge, styling + 6 palettes - Radar: default, circle, filled+values, colorblind + 6 palettes - Sankey: horizontal, vertical + 6 palettes - Treemap: default, with values + 6 palettes - Sunburst: default, no labels + 6 palettes - Content: markdown (with tables), JSON viewer, iframe - Detail: click action target page Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When onClick is set, CrossFilterTag renders as a <button>. The remove control inside was also a <button>, creating invalid nested buttons that cause React hydration errors. Fix: use <span role="button"> with keyboard handlers when the outer element is a button. Native <button> is preserved when outer is a div. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 6 new pages (18 total) to the Chart Catalog seed dashboard: - Page 12: Graph — force/circular/hierarchical, node sizes, labels, physics - Page 13: Parameter Widgets — select (searchable/not), text, date, date-range, relative-date - Page 14: Form Widget — default form, custom button + no-reset - Page 15: Behavior — showRefreshButton, manualRun, cacheMode (forever/TTL) - Page 16: Axis & Grid — axis labels, grid lines off, bar width/gap, legend off, sorted slices, no percentages - Page 17: Advanced — table pagination/pageSize, gauge min/max/progress/detail, radar legend, sankey nodeWidth/gap, sunburst sort/highlight, treemap breadcrumb/saturation, JSON fontSize/theme/copyButton Every chart option from chart-options-schema.ts now has at least one widget demonstrating it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ces) Add FILMED_IN and BORN_IN relationships to Neo4j seed connecting Movies and Persons to City nodes (which already had lat/lng coordinates). New Map Chart page in Chart Catalog (6 widgets): - Cities by population (OSM) - Filming locations (Carto Light) - Birthplaces (Carto Dark) - Clustered markers - Custom zoom / no popup - Large markers with click action All map options covered: tileLayer (osm/carto-light/carto-dark), autoFitBounds, markerSize, showPopup, clusterMarkers, zoom, minZoom, maxZoom. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Markdown: use actual \n newlines instead of escaped \\n literals - iFrame: replace echarts.apache.org (blocks framing via X-Frame-Options) with Wikipedia which allows embedding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add GFM table parsing to markdown-widget.tsx on this branch (was only on feat/issue-143-markdown-tables branch) - Reduce graph widgets from 6 to 4, use smaller queries (LIMIT 10) to prevent NVL physics engine overload with simultaneous renders - Fix graphSmall query: bind relationship variable properly - Type annotations for parseCells in markdown parser Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Gate NVL canvas visibility with a layoutReady state that flips true only when onLayoutDone fires. Reset is synchronous during render (not useEffect) to avoid a race where the effect runs after onLayoutDone on the main thread. Replace the arbitrary 100ms autoFit timer with a deterministic layoutReady guard. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add shared formatNumber() and buildTooltipFormatter() to chart-utils.ts: - Decimal places config (0-6, -1 for automatic) - Comma/compact/percent formatting - Prefix/suffix support - Consistent tooltip formatting across all ECharts chart types Add decimalPlaces option to single-value, bar, line, and pie chart schemas. Update SingleValueChart to use the shared formatter. Closes #138 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Apply userAria spread before ariaDescription so the explicit description takes precedence over user-provided aria options - Add ariaDescription to useEffect dependency array to re-apply when the description changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Radar chart: only store explicit max when finite and > 0, so invalid/null/NaN values fall back to globalMax instead of being treated as explicit 100. - Seed: move colorPalette to settings.colorPalette (matches chart-renderer.tsx), fix navigateToPageId -> targetPageId. - Markdown: replace ReDoS-vulnerable table alignment regex with linear split-and-check function (isTableAlignmentRow). - Add tests for radar invalid max fallback and GFM table rendering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The typeInEditor fixture was retrying until timeout when CM6's internal view.state.readOnly was true. Now falls through to the keyboard fallback strategy instead of throwing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unused NumberFormatConfig type import in format-number.test.ts that caused TS6133 error during CI type-check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… into release/a11y-and-fixes
alfredo1996
added
enhancement
WalkthroughThis PR introduces number formatting utilities, updates chart axis scaling, enhances accessibility with ARIA attributes and semantic HTML, refactors graph chart layout timing, adds GFM table support to markdown parsing, and extends the demo seed script with a Chart Catalog dashboard spanning multiple chart types. Changes
Sequence DiagramsequenceDiagram
participant Component as GraphChart<br/>Component
participant State as layoutReady<br/>State
participant NVL as NVL Library
participant Callback as onLayoutDone<br/>Callback
participant Fit as fitGraph()<br/>Function
Component->>State: Initial render<br/>(layoutReady=false)
Component->>NVL: Render with nodes
NVL->>NVL: Perform layout calculation
NVL->>Callback: Fire onLayoutDone event
Callback->>Fit: Call fitGraph()
Callback->>State: Set layoutReady=true
Component->>Component: Remove loading overlay
Note over Component,Fit: On nodes change:<br/>layoutReady reset to false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/lib/chart-registry.ts (1)
491-494:⚠️ Potential issue | 🟠 MajorIgnore non-finite radar values before deriving the shared scale.
This hardens invalid
maxinputs, but it still trusts invalidvalueinputs.Number(...)preservesInfinity, so one bad cell can turnglobalMax/wideMaxintoInfinityand poison the whole chart scale.Suggested fix
function transformToRadarData(data: unknown): unknown { const records = toRecords(data); if (!records.length) return { indicators: [], series: [] }; + const toFiniteNumber = (value: unknown): number => { + const n = Number(value); + return Number.isFinite(n) ? n : 0; + }; + const keys = Object.keys(records[0]); const indicatorKey = keys.find((k) => /^(indicator|axis|dimension|category)$/i.test(k)); const valueKey = keys.find((k) => /^(value|score)$/i.test(k) && k !== indicatorKey); const maxKey = keys.find((k) => /^(max|maximum)$/i.test(k)); const seriesKey = keys.find((k) => /^(series|group|name|label)$/i.test(k) && k !== indicatorKey && k !== valueKey && k !== maxKey); @@ for (const r of records) { const indName = String(normalizeValue(r[indicatorKey]) ?? ""); - const val = Number(r[valueKey]) || 0; + const val = toFiniteNumber(r[valueKey]); const serName = seriesKey ? String(normalizeValue(r[seriesKey]) ?? "Default") : "Default"; @@ for (const r of records) { for (const k of keys) { - const v = Number(r[k]) || 0; + const v = toFiniteNumber(r[k]); if (v > wideGlobalMax) wideGlobalMax = v; } } @@ const series = records.map((r, i) => ({ name: String(i + 1), - values: keys.map((k) => Number(r[k]) || 0), + values: keys.map((k) => toFiniteNumber(r[k])), }));Also applies to: 507-513, 523-539
🤖 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 491 - 494, The code converts values with Number(...) which preserves Infinity/NaN and can poison scale calculations; update the record-processing loops that compute and aggregate values (the loop creating indName/val/serName and any subsequent loops that update globalMax and wideMax) to coerce values via Number(...) then guard with Number.isFinite(val) before using them — skip or treat non-finite values as absent (do not let them contribute to globalMax/wideMax or series min/max), and ensure normalizeValue outputs are also checked for finiteness where used; apply the same finite-value check to the other similar loops that derive shared scales so Infinity/NaN are ignored.component/src/charts/base-chart.tsx (1)
153-166:⚠️ Potential issue | 🟠 MajorDon't overwrite
options.aria.labelwhenariaDescriptionis set.This spread replaces the entire
labelobject. Any caller-suppliedaria.label.general,series,data, etc. disappears as soon asariaDescriptionis passed. Deep-mergeuserAria.labeland override onlydescription.Suggested fix
const userAria = (options?.aria ?? {}) as Record<string, unknown>; const userDecal = (userAria.decal ?? {}) as Record<string, unknown>; + const userLabel = (userAria.label ?? {}) as Record<string, unknown>; const merged: EChartsOption = { color: resolvedColors, ...options, aria: { enabled: true, ...userAria, - ...(ariaDescription ? { label: { description: ariaDescription } } : {}), + ...(ariaDescription + ? { label: { ...userLabel, description: ariaDescription } } + : {}), decal: { show: colorblindMode, ...userDecal }, }, };🤖 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 153 - 166, The current merge for aria replaces the entire user-provided aria.label when ariaDescription is present; update the merge to preserve caller-supplied label fields by extracting userAria.label (e.g., const userLabel = (userAria.label ?? {}) as Record<string, unknown>) and then spread userLabel into the final aria.label while only overriding/setting description to ariaDescription, leaving other aria properties (userAria, userDecal) intact before calling instance.setOption with merged.
🧹 Nitpick comments (4)
scripts/seed-demo.mjs (2)
2027-2034: Unused helper function — dead code.
clickNavPageis defined but never called. Either remove it or wire it up to widgets that should navigate todetailPageId/behaviorPageId.🗑️ Remove if not needed
- // Click action: navigate to page - const clickNavPage = (triggerCol, pageId) => ({ - type: "navigate-to-page", - rules: [{ - id: uuid(), type: "navigate-to-page", - triggerColumn: triggerCol, - targetPageId: pageId, - }], - });🤖 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, clickNavPage is defined but never used; either remove the dead helper or wire it into the widget definitions that should perform navigation (e.g., attach its return value to those widgets' behavior/rules). Locate the clickNavPage function and either delete it, or replace the unused inline rule objects for navigating to detailPageId/behaviorPageId by calling clickNavPage(triggerColumn, detailPageId) or clickNavPage(triggerColumn, behaviorPageId) where appropriate (ensure uuid() and triggerColumn variables are provided).
2578-2594: Detail page is orphaned — no navigation leads here.The "Detail View" page expects
$param_bar_decade(set by Bar Chart click action), but no widget actually navigates to this page. Users must manually navigate. If this is intentional as a navigation target, consider wiring upclickNavPage(detailPageId)on the Bar Chart page. Otherwise, removedetailPageIdand this page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/seed-demo.mjs` around lines 2578 - 2594, The Detail View page (detailPageId) is orphaned and expects $param_bar_decade but no widget navigates to it; either wire the bar chart's click action to navigate to this page by adding clickNavPage(detailPageId) on the bar chart widget (the widget that sets $param_bar_decade), ensuring the bar chart's click action also sets the parameter used by the Detail View, or remove detailPageId and the entire Detail View page if you don't intend it as a navigation target; update the Bar Chart page's widget configuration (the bar chart widget that currently sets $param_bar_decade) to call clickNavPage(detailPageId) when clicked.component/src/charts/graph-chart.tsx (1)
578-585: Announce the new layout overlay to assistive tech.Right now this spinner is purely visual. Adding a status role plus hidden text would make the relayout state discoverable for screen-reader users too.
Suggested tweak
{!layoutReady && nodes.length > 0 && ( <div className="absolute inset-0 z-20 flex items-center justify-center bg-background/60 backdrop-blur-[1px]" data-testid="graph-loading-overlay" + role="status" + aria-live="polite" > + <span className="sr-only">Loading graph layout</span> <div className="h-5 w-5 animate-spin rounded-full border-2 border-primary border-t-transparent" /> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/graph-chart.tsx` around lines 578 - 585, The overlay that renders when !layoutReady && nodes.length > 0 (the element with data-testid="graph-loading-overlay") is only visual; make it accessible by adding role="status" and a visually-hidden live message (e.g., a span with screen-reader-only class) that announces the relayout like "Graph layout updating" so screen readers detect the state change; update the JSX for the overlay container (the same element using layoutReady/nodes and data-testid="graph-loading-overlay") to include these attributes and hidden text.component/src/charts/chart-utils.ts (1)
79-81: Edge case: array value with fewer than 2 elements.If
p.valueis an array with only one element (e.g.,[100]),p.value[1]returnsundefined, which would render as the string"undefined"in the tooltip. This is unlikely with standard ECharts data shapes but worth a defensive check if you want robustness.🛡️ Optional defensive fix
- const raw = Array.isArray(p.value) ? p.value[1] : p.value; + const raw = Array.isArray(p.value) ? (p.value[1] ?? p.value[0]) : p.value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/chart-utils.ts` around lines 79 - 81, The tooltip value extraction in chart-utils.ts uses Array.isArray(p.value) ? p.value[1] : p.value which returns undefined for single-element arrays and can render "undefined"; update the extraction logic inside the items.map callback (the raw variable used with formatNumber and tooltipConfig) to defensively pick p.value[1] when present, otherwise fall back to p.value[0] (or the scalar p.value) and then coalesce undefined to an empty string before formatting/converting to String so tooltips never show "undefined".
🤖 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 497-500: The code currently keeps the first seen valid explicitMax
for an indicator (using indicatorExplicitMax.has(indName)), causing
non-deterministic axes when multiple rows provide different valid maxima; change
the logic in the block that reads explicitMax from r[maxKey] so that for a valid
Number.isFinite(explicitMax) && explicitMax > 0 you either set the map entry
when absent or update it deterministically by taking the maximum of the existing
value and explicitMax (e.g., read existing = indicatorExplicitMax.get(indName)
and indicatorExplicitMax.set(indName, existing ? Math.max(existing, explicitMax)
: explicitMax)), keeping the same validation checks around explicitMax and using
the same symbols indName, explicitMax, r[maxKey], and indicatorExplicitMax.
In `@component/src/charts/graph-chart.tsx`:
- Around line 305-312: The layout gate is reset only when the nodes array
identity changes, causing desync with onLayoutDone which currently calls
fitGraph() unconditionally; instead, reset layoutReady whenever a fresh layout
run starts (e.g., when nodes, edges, or layout/physics props change or when you
invoke the layout start path) by updating the prevNodesRef logic (and equivalent
refs for edges/layout props) to setLayoutReady(false) at layout start, and
remove the unconditional fitGraph() call from onLayoutDone so the existing
effect that watches layoutReady + autoFit performs the actual fit; update
references in the blocks around layoutReady/prevNodesRef, onLayoutDone, and the
autoFit effect to ensure only the autoFit effect centers the graph.
In `@component/src/components/composed/chart-options-schema.ts`:
- Around line 15-18: The schema's tooltipFormatOptions currently defaults
decimalPlaces to -1 which later flows through chart-options-panel.tsx unchanged
and causes buildTooltipFormatter()/formatNumber() to call toFixed()/Intl options
with invalid negative values; fix by changing the tooltipFormatOptions entry for
"decimalPlaces" to use a safe default (e.g., 0 or undefined) or clamp/normalize
the value to the allowed range (0–6) before it is forwarded—e.g., in
chart-options-panel.tsx normalize Number(e.target.value) so -1 becomes undefined
or is clamped to 0–6, and ensure buildTooltipFormatter()/formatNumber() treat
undefined as automatic formatting (matching single-value-chart.tsx's
decimalPlaces >= 0 guard).
In `@component/src/components/composed/code-preview.tsx`:
- Line 37: The change to the CSS class in CodePreview
(component/src/components/composed/code-preview.tsx) removed opacity which
alters rendered contrast; add automated visual evidence by capturing screenshots
of the component before and after this change and commit them to
.screenshots/before/ and .screenshots/after/ respectively, naming files to
identify the component (e.g., code-preview.png); ensure the screenshots cover
the exact rendered state that changed (run the local renderer/storybook or the
same test harness used by the repo), include any viewport or theme settings
used, and update the PR with those two images so they satisfy the
{app,component}/src/**/*.{ts,tsx} screenshot requirement.
In `@component/src/components/composed/cross-filter-tag.tsx`:
- Around line 31-57: The current removeControl logic renders a focusable <span
role="button" tabIndex={0}> inside an outer <button> when both onClick and
onRemove are provided, which creates invalid nested interactive elements; update
the rendering so when onClick and onRemove coexist you render the clickable item
and the remove control as sibling native <button> elements (not a focusable span
inside a button), e.g. wrap them in a non-interactive container with
role="group" and place the remove action as its own <button type="button"> that
stops propagation and calls onRemove; adjust the conditional in removeControl
(and the parent render that uses onClick) so removeControl produces a native
button sibling when onClick is present, preserving the existing
stopPropagation/onRemove behavior and the X icon usage.
In `@component/src/components/composed/json-viewer.tsx`:
- Around line 82-85: The node is still announced as expandable even when isEmpty
is true; update the interactive behavior around the toggle so empty nodes are
not exposed as expandable controls: when isEmpty is true, remove the onClick
handler and the aria-expanded attribute, avoid Expand/Collapse wording in
aria-label (use a static label like the type or keyName only), and ensure the
element is not keyboard-focusable (e.g., don't render as a button or set
tabIndex to -1); apply this logic where setExpanded, expanded, keyName, type and
isEmpty are used to render the toggle so only non-empty nodes keep onClick,
aria-expanded and the Expand/Collapse aria-label.
In `@component/src/components/composed/markdown-widget.tsx`:
- Around line 36-48: isTableAlignmentRow currently collapses empty cells and
doesn't verify the delimiter count matches headers; update isTableAlignmentRow
to preserve true empty cells when splitting a pipe-delimited row (do not filter
out empty strings produced by leading/trailing or consecutive pipes) and
validate each non-empty cell against the delimiter pattern /^:?-{3,}:?$/ while
also returning false if the number of delimiter cells (after trimming) does not
match the expected header cell count provided by the caller; adjust callers (the
table-detection logic that calls isTableAlignmentRow) to pass the header cell
count (e.g., from the parsed header row) so you can compare widths before
treating a row as a table delimiter.
- Around line 153-160: Headers and table cells are being wrapped with escapeHtml
which prevents inline markdown (bold, code, links) from being rendered; replace
escapeHtml usage with the existing processInline call so inline markdown stays
active while still escaping raw HTML. Update the header loop (where
escapeHtml(h) is used) to use processInline(h) and the cell rendering (where
escapeHtml(row[c] ?? "") is used) to use processInline(row[c] ?? "") so headers
and bodyRows render inline markdown correctly while preserving XSS protections.
In `@scripts/seed-demo.mjs`:
- Around line 2380-2402: The trailing comment after the Graph Chart block
incorrectly repeats "Page 13"; update the comment that follows this widget
object so it reads "Page 14: Parameter Widgets" instead of "Page 13: Parameter
Widgets" to keep page numbering consistent — look for the block with title
"Graph Chart" and the subsequent comment line and change its page number.
---
Outside diff comments:
In `@app/src/lib/chart-registry.ts`:
- Around line 491-494: The code converts values with Number(...) which preserves
Infinity/NaN and can poison scale calculations; update the record-processing
loops that compute and aggregate values (the loop creating indName/val/serName
and any subsequent loops that update globalMax and wideMax) to coerce values via
Number(...) then guard with Number.isFinite(val) before using them — skip or
treat non-finite values as absent (do not let them contribute to
globalMax/wideMax or series min/max), and ensure normalizeValue outputs are also
checked for finiteness where used; apply the same finite-value check to the
other similar loops that derive shared scales so Infinity/NaN are ignored.
In `@component/src/charts/base-chart.tsx`:
- Around line 153-166: The current merge for aria replaces the entire
user-provided aria.label when ariaDescription is present; update the merge to
preserve caller-supplied label fields by extracting userAria.label (e.g., const
userLabel = (userAria.label ?? {}) as Record<string, unknown>) and then spread
userLabel into the final aria.label while only overriding/setting description to
ariaDescription, leaving other aria properties (userAria, userDecal) intact
before calling instance.setOption with merged.
---
Nitpick comments:
In `@component/src/charts/chart-utils.ts`:
- Around line 79-81: The tooltip value extraction in chart-utils.ts uses
Array.isArray(p.value) ? p.value[1] : p.value which returns undefined for
single-element arrays and can render "undefined"; update the extraction logic
inside the items.map callback (the raw variable used with formatNumber and
tooltipConfig) to defensively pick p.value[1] when present, otherwise fall back
to p.value[0] (or the scalar p.value) and then coalesce undefined to an empty
string before formatting/converting to String so tooltips never show
"undefined".
In `@component/src/charts/graph-chart.tsx`:
- Around line 578-585: The overlay that renders when !layoutReady &&
nodes.length > 0 (the element with data-testid="graph-loading-overlay") is only
visual; make it accessible by adding role="status" and a visually-hidden live
message (e.g., a span with screen-reader-only class) that announces the relayout
like "Graph layout updating" so screen readers detect the state change; update
the JSX for the overlay container (the same element using layoutReady/nodes and
data-testid="graph-loading-overlay") to include these attributes and hidden
text.
In `@scripts/seed-demo.mjs`:
- Around line 2027-2034: clickNavPage is defined but never used; either remove
the dead helper or wire it into the widget definitions that should perform
navigation (e.g., attach its return value to those widgets' behavior/rules).
Locate the clickNavPage function and either delete it, or replace the unused
inline rule objects for navigating to detailPageId/behaviorPageId by calling
clickNavPage(triggerColumn, detailPageId) or clickNavPage(triggerColumn,
behaviorPageId) where appropriate (ensure uuid() and triggerColumn variables are
provided).
- Around line 2578-2594: The Detail View page (detailPageId) is orphaned and
expects $param_bar_decade but no widget navigates to it; either wire the bar
chart's click action to navigate to this page by adding
clickNavPage(detailPageId) on the bar chart widget (the widget that sets
$param_bar_decade), ensuring the bar chart's click action also sets the
parameter used by the Detail View, or remove detailPageId and the entire Detail
View page if you don't intend it as a navigation target; update the Bar Chart
page's widget configuration (the bar chart widget that currently sets
$param_bar_decade) to call clickNavPage(detailPageId) when clicked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab63436f-2e22-405d-8d36-8a8fc55480ec
⛔ Files ignored due to path filters (1)
docker/neo4j/init.cypheris excluded by!docker/**
📒 Files selected for processing (22)
app/e2e/fixtures.tsapp/src/lib/__tests__/chart-registry.test.tsapp/src/lib/chart-registry.tscomponent/src/charts/__tests__/base-chart.test.tsxcomponent/src/charts/__tests__/format-number.test.tscomponent/src/charts/__tests__/graph-chart.test.tsxcomponent/src/charts/__tests__/single-value-chart.test.tsxcomponent/src/charts/base-chart.tsxcomponent/src/charts/chart-utils.tscomponent/src/charts/graph-chart.tsxcomponent/src/charts/single-value-chart.tsxcomponent/src/charts/types.tscomponent/src/components/composed/__tests__/markdown-widget.test.tsxcomponent/src/components/composed/__tests__/query-editor.test.tsxcomponent/src/components/composed/chart-options-schema.tscomponent/src/components/composed/code-preview.tsxcomponent/src/components/composed/cross-filter-tag.tsxcomponent/src/components/composed/json-viewer.tsxcomponent/src/components/composed/markdown-widget.tsxcomponent/src/components/composed/query-editor.tsxcomponent/vitest.setup.tsscripts/seed-demo.mjs
Comment on lines
+497
to
+500
| const explicitMax = Number(r[maxKey]); | ||
| if (Number.isFinite(explicitMax) && explicitMax > 0 && !indicatorExplicitMax.has(indName)) { | ||
| indicatorExplicitMax.set(indName, explicitMax); | ||
| } |
There was a problem hiding this comment.
Make explicit radar maxima deterministic across repeated indicators.
The new !indicatorExplicitMax.has(indName) check means the first valid max wins. In long-format radar data, the same indicator usually appears once per series, so conflicting valid maxima now make the rendered axis depend on row order.
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),
+ );
}
}🤖 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 497 - 500, The code currently
keeps the first seen valid explicitMax for an indicator (using
indicatorExplicitMax.has(indName)), causing non-deterministic axes when multiple
rows provide different valid maxima; change the logic in the block that reads
explicitMax from r[maxKey] so that for a valid Number.isFinite(explicitMax) &&
explicitMax > 0 you either set the map entry when absent or update it
deterministically by taking the maximum of the existing value and explicitMax
(e.g., read existing = indicatorExplicitMax.get(indName) and
indicatorExplicitMax.set(indName, existing ? Math.max(existing, explicitMax) :
explicitMax)), keeping the same validation checks around explicitMax and using
the same symbols indName, explicitMax, r[maxKey], and indicatorExplicitMax.
Comment on lines
+305
to
+312
| // Reset layoutReady synchronously during render when nodes change. | ||
| // Using useEffect would race with onLayoutDone (which fires before | ||
| // effects run when the simulation completes on the main thread). | ||
| const prevNodesRef = useRef(nodes); | ||
| if (prevNodesRef.current !== nodes) { | ||
| prevNodesRef.current = nodes; | ||
| if (layoutReady) setLayoutReady(false); | ||
| } |
There was a problem hiding this comment.
The layout gate and autoFit are out of sync.
layoutReady only resets when the nodes array identity changes, but onLayoutDone now calls fitGraph() unconditionally. After a layout / physics / edge change the gate can stay stale, autoFit={false} still recenters, and autoFit={true} can recenter twice. Reset the gate whenever a fresh layout starts, and let the existing autoFit effect own the actual fit.
Suggested direction
- const prevNodesRef = useRef(nodes);
- if (prevNodesRef.current !== nodes) {
- prevNodesRef.current = nodes;
+ const prevLayoutInputsRef = useRef({ nodes, edges, layout, physics });
+ if (
+ prevLayoutInputsRef.current.nodes !== nodes ||
+ prevLayoutInputsRef.current.edges !== edges ||
+ prevLayoutInputsRef.current.layout !== layout ||
+ prevLayoutInputsRef.current.physics !== physics
+ ) {
+ prevLayoutInputsRef.current = { nodes, edges, layout, physics };
if (layoutReady) setLayoutReady(false);
}
@@
const nvlCallbacks = useMemo(
() => ({
onLayoutDone: () => {
- fitGraph();
setLayoutReady(true);
},
}),
- [fitGraph],
+ [],
);Also applies to: 387-393, 437-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/charts/graph-chart.tsx` around lines 305 - 312, The layout gate
is reset only when the nodes array identity changes, causing desync with
onLayoutDone which currently calls fitGraph() unconditionally; instead, reset
layoutReady whenever a fresh layout run starts (e.g., when nodes, edges, or
layout/physics props change or when you invoke the layout start path) by
updating the prevNodesRef logic (and equivalent refs for edges/layout props) to
setLayoutReady(false) at layout start, and remove the unconditional fitGraph()
call from onLayoutDone so the existing effect that watches layoutReady + autoFit
performs the actual fit; update references in the blocks around
layoutReady/prevNodesRef, onLayoutDone, and the autoFit effect to ensure only
the autoFit effect centers the graph.
Comment on lines
+15
to
+18
| /** Shared number formatting options for tooltip values on axis-based charts. */ | ||
| const tooltipFormatOptions: ChartOptionDef[] = [ | ||
| { key: "decimalPlaces", label: "Decimal Places", type: "number", default: -1, category: "Labels", description: "Fixed number of decimal places in tooltips (0-6). Set to -1 for automatic." }, | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'decimalPlaces' \
component/src/components/composed/chart-options-panel.tsx \
component/src/charts/chart-utils.ts \
component/src/charts/single-value-chart.tsxRepository: alfredo1996/neoboard
Length of output: 3692
🏁 Script executed:
# Check chart-options-panel.tsx for how it handles decimalPlaces
rg -n -A5 -B5 'decimalPlaces' component/src/components/composed/chart-options-panel.tsxRepository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Find all calls to formatNumber to identify vulnerable consumers
rg -n 'formatNumber\s*\(' --type ts --type tsx component/src/Repository: alfredo1996/neoboard
Length of output: 91
🏁 Script executed:
# Check for tooltip-related code that uses decimalPlaces or formatNumber
rg -n 'tooltip.*formatNumber|formatNumber.*tooltip|tooltipFormatOptions' --type ts --type tsx component/src/Repository: alfredo1996/neoboard
Length of output: 91
🏁 Script executed:
# List the files we need to inspect
fd -t f '(chart-options-panel|chart-utils|single-value-chart)' component/src/Repository: alfredo1996/neoboard
Length of output: 333
🏁 Script executed:
# Search for decimalPlaces usage with correct rg syntax (no tsx type)
rg -n 'decimalPlaces' -t ts component/src/components/composed/chart-options-panel.tsxRepository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
# Find all formatNumber calls
rg -n 'formatNumber\s*\(' -t ts component/src/Repository: alfredo1996/neoboard
Length of output: 2443
🏁 Script executed:
# Read chart-options-panel.tsx to see how decimalPlaces is handled
cat -n component/src/components/composed/chart-options-panel.tsx | head -200Repository: alfredo1996/neoboard
Length of output: 6689
🏁 Script executed:
# Read chart-utils.ts around the tooltip code (lines 70-90)
cat -n component/src/charts/chart-utils.ts | sed -n '70,90p'Repository: alfredo1996/neoboard
Length of output: 1203
🏁 Script executed:
# Also check if tooltipFormatOptions is referenced in chart-options-panel
rg -n 'tooltipFormatOptions' -t ts component/src/Repository: alfredo1996/neoboard
Length of output: 709
Negative decimalPlaces default breaks tooltip formatting in bar/line/pie charts.
The schema defaults decimalPlaces to -1 (line 16), and chart-options-panel.tsx (line 120) passes numeric input unchanged via Number(e.target.value). This gets forwarded to buildTooltipFormatter() → formatNumber() (line 81 in chart-utils.ts), which calls toFixed(-1), minimumFractionDigits: -1, or similar APIs that require [0, 20] and will throw or produce invalid output. single-value-chart.tsx already guards this locally (line 91: decimalPlaces >= 0), proving the problem exists. Clamp decimalPlaces to 0-6 in the schema or normalize -1 → undefined in the panel/formatter layer before it reaches these APIs.
Also applies to: 81, 373–375
🤖 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 15 -
18, The schema's tooltipFormatOptions currently defaults decimalPlaces to -1
which later flows through chart-options-panel.tsx unchanged and causes
buildTooltipFormatter()/formatNumber() to call toFixed()/Intl options with
invalid negative values; fix by changing the tooltipFormatOptions entry for
"decimalPlaces" to use a safe default (e.g., 0 or undefined) or clamp/normalize
the value to the allowed range (0–6) before it is forwarded—e.g., in
chart-options-panel.tsx normalize Number(e.target.value) so -1 becomes undefined
or is clamped to 0–6, and ensure buildTooltipFormatter()/formatNumber() treat
undefined as automatic formatting (matching single-value-chart.tsx's
decimalPlaces >= 0 guard).
| > | ||
| {language && ( | ||
| <span className="absolute top-1 right-1.5 text-[9px] font-medium text-muted-foreground/50 uppercase select-none pointer-events-none"> | ||
| <span className="absolute top-1 right-1.5 text-[9px] font-medium text-muted-foreground uppercase select-none pointer-events-none"> |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Verify screenshot artifacts exist for visual changes in app/component src files.
# Expected: at least one matching file in both directories.
fd . .screenshots/before
fd . .screenshots/afterRepository: alfredo1996/neoboard
Repository: alfredo1996/neoboard
Exit code: 1
stderr:
[fd error]: Search path '.screenshots/before' is not a directory.
[fd error]: No valid search paths given.
[fd error]: Search path '.screenshots/after' is not a directory.
[fd error]: No valid search paths given.
Script executed:
#!/bin/bash
# Verify the file exists
echo "=== Checking if file exists ==="
ls -la component/src/components/composed/code-preview.tsx 2>/dev/null || echo "File not found"
# Check the actual code around line 37
echo -e "\n=== Examining code around line 37 ==="
sed -n '30,45p' component/src/components/composed/code-preview.tsx 2>/dev/null || echo "Unable to read file"
# Search for any screenshot directories anywhere in repo
echo -e "\n=== Searching for screenshot directories ==="
find . -type d -name "*screenshot*" 2>/dev/null | head -20
# Check if .screenshots directory exists anywhere
echo -e "\n=== Checking for .screenshots directory ==="
ls -la .screenshots 2>/dev/null || echo ".screenshots directory not found"
# List top-level directories to understand repo structure
echo -e "\n=== Repository structure ==="
ls -la | grep -E "^d"Repository: alfredo1996/neoboard
Repository: alfredo1996/neoboard
Exit code: 0
stdout:
=== Checking if file exists ===
-rw-r--r-- 1 jailuser jailuser 2027 Mar 24 12:01 component/src/components/composed/code-preview.tsx
=== Examining code around line 37 ===
data-testid="code-preview"
className={cn(
"relative rounded-md bg-muted/40 overflow-hidden",
className,
)}
>
{language && (
<span className="absolute top-1 right-1.5 text-[9px] font-medium text-muted-foreground uppercase select-none pointer-events-none">
{language}
</span>
)}
<pre className={cn("px-2.5 py-2 overflow-hidden", !expanded && "h-full")}>
<code className="text-[11px] leading-snug text-muted-foreground font-mono whitespace-pre-wrap break-all">
{displayText}
</code>
</pre>
=== Searching for screenshot directories ===
./app/.screenshots
./.claude/skills/screenshot-review
=== Checking for .screenshots directory ===
.screenshots directory not found
=== Repository structure ===
drwxr-xr-x 13 jailuser jailuser 600 Mar 24 12:02 .
drwxr-xr-x 1 jailuser jailuser 240 Mar 24 12:01 ..
drwxr-xr-x 5 jailuser jailuser 140 Mar 24 12:02 .claude
drwxr-xr-x 7 jailuser jailuser 280 Mar 24 12:06 .git
drwxr-xr-x 3 jailuser jailuser 80 Mar 24 12:01 .github
drwxr-xr-x 7 jailuser jailuser 380 Mar 24 12:02 app
drwxr-xr-x 3 jailuser jailuser 80 Mar 24 12:02 claude_code_docs
drwxr-xr-x 5 jailuser jailuser 320 Mar 24 12:01 component
drwxr-xr-x 4 jailuser jailuser 220 Mar 24 12:01 connection
drwxr-xr-x 4 jailuser jailuser 100 Mar 24 12:02 docker
drwxr-xr-x 4 jailuser jailuser 200 Mar 24 12:02 docs
drwxr-xr-x 2 jailuser jailuser 100 Mar 24 12:01 scripts
drwxr-xr-x 3 jailuser jailuser 80 Mar 24 12:01 stress
Script executed:
#!/bin/bash
# Check what's in app/.screenshots
echo "=== Contents of app/.screenshots ==="
ls -la app/.screenshots 2>/dev/null || echo "app/.screenshots not accessible"
find app/.screenshots -type f 2>/dev/null | head -20
# Check if there are any .screenshots or screenshots directories anywhere in repo
echo -e "\n=== All screenshot-related directories ==="
find . -type d \( -name "*screenshot*" -o -name ".screenshots" \) 2>/dev/nullRepository: alfredo1996/neoboard
Repository: alfredo1996/neoboard
Exit code: 0
stdout:
=== Contents of app/.screenshots ===
total 44
drwxr-xr-x 4 jailuser jailuser 100 Mar 24 12:02 .
drwxr-xr-x 7 jailuser jailuser 380 Mar 24 12:02 ..
drwxr-xr-x 2 jailuser jailuser 80 Mar 24 12:02 after
drwxr-xr-x 2 jailuser jailuser 140 Mar 24 12:02 before
-rw-r--r-- 1 jailuser jailuser 42599 Mar 24 12:02 form-widget-403-write-permission.png
app/.screenshots/after/api-keys-create-dialog.png
app/.screenshots/after/api-keys-populated-table.png
app/.screenshots/before/api-keys-create-dialog.png
app/.screenshots/before/api-keys-created-dialog.png
app/.screenshots/before/api-keys-empty-state.png
app/.screenshots/before/api-keys-populated-table.png
app/.screenshots/before/api-keys-revoke-confirm.png
app/.screenshots/form-widget-403-write-permission.png
=== All screenshot-related directories ===
./app/.screenshots
./.claude/skills/screenshot-review
Add before/after screenshots for this visual change.
The opacity removal in this class affects rendered contrast. Per NeoBoard guidelines, {app,component}/src/**/*.{ts,tsx} files with visual changes require screenshot evidence in .screenshots/before/ and .screenshots/after/ directories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/code-preview.tsx` at line 37, The change to
the CSS class in CodePreview
(component/src/components/composed/code-preview.tsx) removed opacity which
alters rendered contrast; add automated visual evidence by capturing screenshots
of the component before and after this change and commit them to
.screenshots/before/ and .screenshots/after/ respectively, naming files to
identify the component (e.g., code-preview.png); ensure the screenshots cover
the exact rendered state that changed (run the local renderer/storybook or the
same test harness used by the repo), include any viewport or theme settings
used, and update the PR with those two images so they satisfy the
{app,component}/src/**/*.{ts,tsx} screenshot requirement.
Comment on lines
+31
to
+57
| // 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> | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can an HTML <button> contain a descendant with tabindex="0" or role="button"? Please cite the WHATWG HTML specification for the <button> content model and ARIA-in-HTML guidance.
💡 Result:
No, an HTML element cannot contain a descendant with tabindex="0" or role="button".
Citations:
- 1: https://html.spec.whatwg.org/multipage/form-elements.html
- 2: https://html.spec.whatwg.org/multipage/button.html
- 3: https://html.spec.whatwg.org/dev/form-elements.html
- 4: https://w3c.github.io/html-reference/aria/button.button.html
- 5: https://www.w3.org/TR/aria-in-html/
- 6: https://w3c.github.io/html-aria/
- 7: https://w3.org/TR/2023/REC-html-aria-20230531
Remove the focusable pseudo-button from inside the outer <button> element.
When both onClick and onRemove are present, the current code renders <span role="button" tabIndex={0}> inside a native <button>. This violates the HTML button content model—buttons cannot contain focusable descendants with role="button" or tabIndex. This breaks keyboard and screen-reader behavior.
Use sibling native buttons instead, wrapping them in a non-interactive div with role="group":
Proposed fix
+ if (onClick && onRemove) {
+ return (
+ <div className={classes} title={tooltip} role="group" aria-label="Cross-filter actions">
+ <button type="button" onClick={onClick} className="inline-flex items-center gap-1.5">
+ <Filter className="h-3 w-3 text-muted-foreground" />
+ <span className="font-medium">{field}</span>
+ <span>=</span>
+ <span className="font-medium">{value}</span>
+ </button>
+ <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>
+ </div>
+ );
+ }
+
if (onClick) {
return (
<button type="button" className={classes} onClick={onClick} title={tooltip}>
{content}
</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 - 57,
The current removeControl logic renders a focusable <span role="button"
tabIndex={0}> inside an outer <button> when both onClick and onRemove are
provided, which creates invalid nested interactive elements; update the
rendering so when onClick and onRemove coexist you render the clickable item and
the remove control as sibling native <button> elements (not a focusable span
inside a button), e.g. wrap them in a non-interactive container with
role="group" and place the remove action as its own <button type="button"> that
stops propagation and calls onRemove; adjust the conditional in removeControl
(and the parent render that uses onClick) so removeControl produces a native
button sibling when onClick is present, preserving the existing
stopPropagation/onRemove behavior and the X icon usage.
Comment on lines
82
to
85
| onClick={() => setExpanded(!expanded)} | ||
| aria-expanded={expanded} | ||
| aria-label={`${expanded ? "Collapse" : "Expand"} ${keyName ?? (type === "array" ? "array" : "object")}`} | ||
| > |
There was a problem hiding this comment.
Don’t expose empty nodes as expandable controls.
Line 83 and Line 84 still announce expand/collapse even when isEmpty is true, but there is no child region to reveal. That creates misleading keyboard/screen-reader behavior.
Suggested fix
- <button
+ <button
type="button"
className="flex items-center cursor-pointer hover:bg-muted/50 rounded-sm w-full text-left"
style={{ paddingLeft: depth * 16 }}
- onClick={() => setExpanded(!expanded)}
- aria-expanded={expanded}
- aria-label={`${expanded ? "Collapse" : "Expand"} ${keyName ?? (type === "array" ? "array" : "object")}`}
+ onClick={!isEmpty ? () => setExpanded(!expanded) : undefined}
+ disabled={isEmpty}
+ aria-expanded={!isEmpty ? expanded : undefined}
+ aria-label={
+ isEmpty
+ ? `${keyName ?? (type === "array" ? "array" : "object")} is empty`
+ : `${expanded ? "Collapse" : "Expand"} ${keyName ?? (type === "array" ? "array" : "object")}`
+ }
>📝 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
| onClick={() => setExpanded(!expanded)} | |
| aria-expanded={expanded} | |
| aria-label={`${expanded ? "Collapse" : "Expand"} ${keyName ?? (type === "array" ? "array" : "object")}`} | |
| > | |
| <button | |
| type="button" | |
| className="flex items-center cursor-pointer hover:bg-muted/50 rounded-sm w-full text-left" | |
| style={{ paddingLeft: depth * 16 }} | |
| onClick={!isEmpty ? () => setExpanded(!expanded) : undefined} | |
| disabled={isEmpty} | |
| aria-expanded={!isEmpty ? expanded : undefined} | |
| aria-label={ | |
| isEmpty | |
| ? `${keyName ?? (type === "array" ? "array" : "object")} is empty` | |
| : `${expanded ? "Collapse" : "Expand"} ${keyName ?? (type === "array" ? "array" : "object")}` | |
| } | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/json-viewer.tsx` around lines 82 - 85, The
node is still announced as expandable even when isEmpty is true; update the
interactive behavior around the toggle so empty nodes are not exposed as
expandable controls: when isEmpty is true, remove the onClick handler and the
aria-expanded attribute, avoid Expand/Collapse wording in aria-label (use a
static label like the type or keyName only), and ensure the element is not
keyboard-focusable (e.g., don't render as a button or set tabIndex to -1); apply
this logic where setExpanded, expanded, keyName, type and isEmpty are used to
render the toggle so only non-empty nodes keep onClick, aria-expanded and the
Expand/Collapse aria-label.
Comment on lines
+36
to
+48
| function isTableAlignmentRow(line: string): boolean { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) return false; | ||
| // Split by pipe, trim each cell, filter out empty leading/trailing cells | ||
| const cells = trimmed.split("|").map((c) => c.trim()); | ||
| // Remove empty strings caused by leading/trailing pipes | ||
| const filtered = cells.filter((c, i) => | ||
| c.length > 0 || (i > 0 && i < cells.length - 1), | ||
| ); | ||
| if (filtered.length === 0) return false; | ||
| // Each non-empty cell must match :?-{3,}:? | ||
| const cellPattern = /^:?-{3,}:?$/; | ||
| return filtered.every((c) => c.length === 0 || cellPattern.test(c)); |
There was a problem hiding this comment.
Preserve real empty cells and validate delimiter width before emitting a table.
This currently collapses blank cells, so | a | | c | shifts c under the wrong header. It also accepts malformed delimiters because blank delimiter cells pass isTableAlignmentRow() and the header width is never compared with the delimiter width.
💡 Proposed fix
+function splitTableCells(row: string): string[] {
+ const trimmed = row.trim();
+ const rawCells = trimmed.split("|").map((c) => c.trim());
+ const start = trimmed.startsWith("|") ? 1 : 0;
+ const end = trimmed.endsWith("|") ? rawCells.length - 1 : rawCells.length;
+ return rawCells.slice(start, end);
+}
+
function isTableAlignmentRow(line: string): boolean {
- const trimmed = line.trim();
- if (!trimmed) return false;
- // Split by pipe, trim each cell, filter out empty leading/trailing cells
- const cells = trimmed.split("|").map((c) => c.trim());
- // Remove empty strings caused by leading/trailing pipes
- const filtered = cells.filter((c, i) =>
- c.length > 0 || (i > 0 && i < cells.length - 1),
- );
- if (filtered.length === 0) return false;
+ const cells = splitTableCells(line);
+ if (cells.length === 0) return false;
// Each non-empty cell must match :?-{3,}:?
const cellPattern = /^:?-{3,}:?$/;
- return filtered.every((c) => c.length === 0 || cellPattern.test(c));
+ return cells.every((c) => cellPattern.test(c));
}
...
+ const headerCells = line.includes("|") ? splitTableCells(line) : [];
+ const delimiterCells =
+ i + 1 < lines.length ? splitTableCells(lines[i + 1]) : [];
if (
- line.includes("|") &&
+ headerCells.length > 0 &&
i + 1 < lines.length &&
- isTableAlignmentRow(lines[i + 1])
+ headerCells.length === delimiterCells.length &&
+ isTableAlignmentRow(lines[i + 1])
) {
closeList();
- const parseCells = (row: string) =>
- row.split("|").map((c: string) => c.trim()).filter((c: string) => c.length > 0);
- const headers = parseCells(line);
+ const headers = headerCells;
i++; // skip alignment row
const bodyRows = [];
while (i + 1 < lines.length && lines[i + 1].includes("|")) {
i++;
- bodyRows.push(parseCells(lines[i]));
+ bodyRows.push(splitTableCells(lines[i]));
}Also applies to: 136-145, 147-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/markdown-widget.tsx` around lines 36 - 48,
isTableAlignmentRow currently collapses empty cells and doesn't verify the
delimiter count matches headers; update isTableAlignmentRow to preserve true
empty cells when splitting a pipe-delimited row (do not filter out empty strings
produced by leading/trailing or consecutive pipes) and validate each non-empty
cell against the delimiter pattern /^:?-{3,}:?$/ while also returning false if
the number of delimiter cells (after trimming) does not match the expected
header cell count provided by the caller; adjust callers (the table-detection
logic that calls isTableAlignmentRow) to pass the header cell count (e.g., from
the parsed header row) so you can compare widths before treating a row as a
table delimiter.
Comment on lines
+153
to
+160
| for (const h of headers) { | ||
| result.push(`<th class="border border-border px-3 py-1.5 font-semibold text-left bg-muted/30">${escapeHtml(h)}</th>`); | ||
| } | ||
| result.push("</tr></thead><tbody>"); | ||
| for (const row of bodyRows) { | ||
| result.push("<tr>"); | ||
| for (let c = 0; c < headers.length; c++) { | ||
| result.push(`<td class="border border-border px-3 py-1.5">${escapeHtml(row[c] ?? "")}</td>`); |
There was a problem hiding this comment.
Keep inline markdown active inside table cells.
Using escapeHtml() here turns **bold**, `code`, and [link](...) into literal text as soon as the block is recognized as a table. That also downgrades links inside tables to plain text. processInline() already escapes raw HTML first, so it preserves the current XSS protections.
💡 Proposed fix
for (const h of headers) {
- result.push(`<th class="border border-border px-3 py-1.5 font-semibold text-left bg-muted/30">${escapeHtml(h)}</th>`);
+ result.push(
+ `<th class="border border-border px-3 py-1.5 font-semibold text-left bg-muted/30">${processInline(h)}</th>`,
+ );
}
...
result.push("<tr>");
for (let c = 0; c < headers.length; c++) {
- result.push(`<td class="border border-border px-3 py-1.5">${escapeHtml(row[c] ?? "")}</td>`);
+ result.push(
+ `<td class="border border-border px-3 py-1.5">${processInline(row[c] ?? "")}</td>`,
+ );
}
result.push("</tr>");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/markdown-widget.tsx` around lines 153 -
160, Headers and table cells are being wrapped with escapeHtml which prevents
inline markdown (bold, code, links) from being rendered; replace escapeHtml
usage with the existing processInline call so inline markdown stays active while
still escaping raw HTML. Update the header loop (where escapeHtml(h) is used) to
use processInline(h) and the cell rendering (where escapeHtml(row[c] ?? "") is
used) to use processInline(row[c] ?? "") so headers and bodyRows render inline
markdown correctly while preserving XSS protections.
Comment on lines
+2380
to
+2402
| // ── Page 13: Graph Chart ───────────────────────────────────── | ||
| { | ||
| id: uuid(), | ||
| title: "Graph Chart", | ||
| widgets: [ | ||
| { id: uuid(), chartType: "graph", connectionId: neo4jId, query: Q.graphData, | ||
| settings: { title: "Force Layout (default)", chartOptions: { layout: "force", showLabels: true, showRelationshipLabels: true, physics: true } } }, | ||
| { id: uuid(), chartType: "graph", connectionId: neo4jId, query: Q.graphSmall, | ||
| settings: { title: "Circular Layout", chartOptions: { layout: "circular", showLabels: true } } }, | ||
| { id: uuid(), chartType: "graph", connectionId: neo4jId, query: Q.graphSmall, | ||
| settings: { title: "Hierarchical", chartOptions: { layout: "hierarchical", showLabels: true } } }, | ||
| { id: uuid(), chartType: "graph", connectionId: neo4jId, query: Q.graphSmall, | ||
| settings: { title: "No Labels / No Physics", chartOptions: { showLabels: false, showRelationshipLabels: false, physics: false, nodeSize: "large" } } }, | ||
| ], | ||
| gridLayout: [ | ||
| { i: null, x: 0, y: 0, w: 6, h: 6 }, | ||
| { i: null, x: 6, y: 0, w: 6, h: 6 }, | ||
| { i: null, x: 0, y: 6, w: 6, h: 6 }, | ||
| { i: null, x: 6, y: 6, w: 6, h: 6 }, | ||
| ], | ||
| }, | ||
|
|
||
| // ── Page 13: Parameter Widgets ───────────────────────────────── |
There was a problem hiding this comment.
Duplicate page number in comments.
Lines 2380 and 2402 both say "Page 13". Should be "Page 13: Graph Chart" and "Page 14: Parameter Widgets".
📝 Fix comments
- // ── Page 13: Graph Chart ─────────────────────────────────────
+ // ── Page 13: Graph Chart ─────────────────────────────────────
{
...
},
- // ── Page 13: Parameter Widgets ─────────────────────────────────
+ // ── Page 14: Parameter Widgets ─────────────────────────────────🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed-demo.mjs` around lines 2380 - 2402, The trailing comment after
the Graph Chart block incorrectly repeats "Page 13"; update the comment that
follows this widget object so it reads "Page 14: Parameter Widgets" instead of
"Page 13: Parameter Widgets" to keep page numbering consistent — look for the
block with title "Graph Chart" and the subsequent comment line and change its
page number.
Owner
Author
|
Merged into PR #185 (release/chart-improvements) for consolidated review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidated batch of 2 quality/accessibility PRs:
Test plan
Closes #101, #103
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements