fix: bracket wrapping in code editor + radar chart scale - #179
Closed
alfredo1996 wants to merge 10 commits into
Closed
fix: bracket wrapping in code editor + radar chart scale#179alfredo1996 wants to merge 10 commits into
alfredo1996 wants to merge 10 commits into
Conversation
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace per-indicator radar auto-scaling with a single global max (observedMax × 1.1, fallback 100) unless explicit per-indicator Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 1
🤖 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 505-513: The code treats missing/invalid explicit max values as an
explicit 100 which then prevents using the computed globalMax; change the logic
so indicatorExplicitMax only contains entries when an explicit/valid numeric max
was actually provided (e.g., check isFinite/max !== null/undefined before set),
and keep missing/invalid entries out (or store undefined) so the indicators
mapping (the indicators array construction using indicatorExplicitMax, maxKey,
globalMax) will fall back to globalMax; update the place that populates
indicatorExplicitMax (not the indicators mapping itself) to perform this
validation so references to indicatorExplicitMax.has(name) truly mean the user
supplied an explicit max.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28893d48-64e5-4690-80bf-0018713af111
📒 Files selected for processing (4)
app/src/lib/__tests__/chart-registry.test.tsapp/src/lib/chart-registry.tscomponent/src/components/composed/__tests__/query-editor.test.tsxcomponent/src/components/composed/query-editor.tsx
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>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/seed-demo.mjs (1)
2018-2024: UsetargetPageIdfor navigation rules to match existing contract.Line 2023 uses
navigateToPageId, but navigate actions elsewhere in this file usetargetPageId(Line 1312, Line 1530). Aligning this avoids latent breakage if this helper is used.Suggested fix
const clickNavPage = (triggerCol, pageId) => ({ type: "navigate-to-page", rules: [{ id: uuid(), type: "navigate-to-page", triggerColumn: triggerCol, - navigateToPageId: pageId, + 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 2018 - 2024, The helper clickNavPage is creating a navigation rule using the property navigateToPageId which is inconsistent with the rest of the codebase; update the rule object inside clickNavPage (type "navigate-to-page", rules array) to use targetPageId instead of navigateToPageId so it matches other navigate actions and the expected contract (keep triggerColumn, id, type unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/seed-demo.mjs`:
- Around line 2028-2035: paletteRow is adding the palette under
settings.chartOptions.colorPalette but existing seed rows expect
settings.colorPalette; update the paletteRow function (symbol: paletteRow) to
set colorPalette at settings.colorPalette (merge baseSettings, set title and
colorPalette on the top-level settings) and keep chartOptions merged from
baseSettings.chartOptions so palette demos use the intended color palette shape.
---
Nitpick comments:
In `@scripts/seed-demo.mjs`:
- Around line 2018-2024: The helper clickNavPage is creating a navigation rule
using the property navigateToPageId which is inconsistent with the rest of the
codebase; update the rule object inside clickNavPage (type "navigate-to-page",
rules array) to use targetPageId instead of navigateToPageId so it matches other
navigate actions and the expected contract (keep triggerColumn, id, type
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea9596a6-afff-4940-9a8d-1b75299934dd
📒 Files selected for processing (1)
scripts/seed-demo.mjs
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
component/src/components/composed/cross-filter-tag.tsx (1)
40-40: Consider splitting the longonKeyDownhandler for readability.The inline handler works but is dense. Extracting to a named handler or splitting across lines would improve scannability.
♻️ Optional: extract handler
+ const handleRemoveKeyDown = (e: React.KeyboardEvent) => { + if (e.key === "Enter" || e.key === " ") { + e.stopPropagation(); + e.preventDefault(); + onRemove?.(); + } + }; + 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(); } }} + onKeyDown={handleRemoveKeyDown} className="ml-1 rounded-full p-0.5 hover:bg-muted cursor-pointer" >🤖 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` at line 40, The inline onKeyDown handler in the CrossFilterTag component is too dense; extract it into a named function (e.g., handleKeyDown) near the component body and attach it as onKeyDown={handleKeyDown}; the handler should check e.key for "Enter" or " " (space), call e.stopPropagation() and e.preventDefault(), then call the existing onRemove() prop; optionally wrap the handler with useCallback to avoid unnecessary re-renders and reference onKeyDown, onRemove, and the CrossFilterTag component when locating where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@component/src/components/composed/cross-filter-tag.tsx`:
- Line 40: The inline onKeyDown handler in the CrossFilterTag component is too
dense; extract it into a named function (e.g., handleKeyDown) near the component
body and attach it as onKeyDown={handleKeyDown}; the handler should check e.key
for "Enter" or " " (space), call e.stopPropagation() and e.preventDefault(),
then call the existing onRemove() prop; optionally wrap the handler with
useCallback to avoid unnecessary re-renders and reference onKeyDown, onRemove,
and the CrossFilterTag component when locating where to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 160b4585-7964-4556-a975-fc3b0a3c4b3a
📒 Files selected for processing (1)
component/src/components/composed/cross-filter-tag.tsx
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>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/seed-demo.mjs (1)
2034-2041:⚠️ Potential issue | 🟠 MajorMove
colorPaletteback to top-level settings.This helper writes the palette to
settings.chartOptions.colorPalette, but the rest of this file seeds palettes atsettings.colorPalette(for example Line 411). That means the catalog palette variants will likely fall back to the default palette.Suggested fix
function paletteRow(chartType, query, baseSettings = {}) { return P.map((p) => ({ id: uuid(), chartType, connectionId: neo4jId, query, - settings: { ...baseSettings, title: p, chartOptions: { ...baseSettings.chartOptions, colorPalette: p } }, + settings: { + ...baseSettings, + title: p, + colorPalette: p, + chartOptions: { ...baseSettings.chartOptions }, + }, })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/seed-demo.mjs` around lines 2034 - 2041, The paletteRow helper is currently writing the palette into settings.chartOptions.colorPalette which is inconsistent with other seed data that uses settings.colorPalette; update paletteRow (the function named paletteRow) to set colorPalette at the top-level of settings instead of under chartOptions—i.e., keep title and existing baseSettings.chartOptions intact but add colorPalette: p directly on the settings object (and remove any chartOptions.colorPalette assignment).
🧹 Nitpick comments (1)
scripts/seed-demo.mjs (1)
2024-2030: UsetargetPageIdin the navigation helper.Every working navigate rule in this file uses
targetPageId(for example Line 1312 and Line 1510). This helper emitsnavigateToPageId, so any future caller will serialize the wrong shape.Suggested fix
const clickNavPage = (triggerCol, pageId) => ({ type: "navigate-to-page", rules: [{ id: uuid(), type: "navigate-to-page", triggerColumn: triggerCol, - navigateToPageId: pageId, + 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 2024 - 2030, The navigation helper clickNavPage is emitting the wrong property name — it currently serializes navigateToPageId for the rule of type "navigate-to-page"; change the emitted property to targetPageId so the rule shape matches other navigate rules (e.g., those at lines referenced) and callers consuming clickNavPage will produce the correct serialized shape; update the object returned by clickNavPage (and any references to navigateToPageId) to use targetPageId while keeping the rule id, type ("navigate-to-page"), triggerColumn, and uuid() usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/seed-demo.mjs`:
- Around line 2034-2041: The paletteRow helper is currently writing the palette
into settings.chartOptions.colorPalette which is inconsistent with other seed
data that uses settings.colorPalette; update paletteRow (the function named
paletteRow) to set colorPalette at the top-level of settings instead of under
chartOptions—i.e., keep title and existing baseSettings.chartOptions intact but
add colorPalette: p directly on the settings object (and remove any
chartOptions.colorPalette assignment).
---
Nitpick comments:
In `@scripts/seed-demo.mjs`:
- Around line 2024-2030: The navigation helper clickNavPage is emitting the
wrong property name — it currently serializes navigateToPageId for the rule of
type "navigate-to-page"; change the emitted property to targetPageId so the rule
shape matches other navigate rules (e.g., those at lines referenced) and callers
consuming clickNavPage will produce the correct serialized shape; update the
object returned by clickNavPage (and any references to navigateToPageId) to use
targetPageId while keeping the rule id, type ("navigate-to-page"),
triggerColumn, and uuid() usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7afe3ff6-d8fa-4067-9eee-6153cadfd606
📒 Files selected for processing (1)
scripts/seed-demo.mjs
…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>
- 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>
|
2 tasks
Owner
Author
|
Superseded by consolidated PR (release/a11y-and-fixes) |
2 tasks
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
Two bug fixes:
1. Parenthesis wrapping in query editor
Selecting text and typing
(now wraps it as(text)instead of replacing it. AddedcloseBrackets()extension andcloseBracketsKeymapfrom@codemirror/autocomplete.2. Radar chart uniform shape
The radar chart was showing a near-uniform pentagon because each indicator had its own independent max. Now uses a single global max so ACTED_IN=172 vs REVIEWED=9 shows actual relative differences.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests