chore: chart polish, E2E speedup, and implementation guide - #599
Merged
Conversation
Gauge — minimal redesign: - Thin arc with rounded progress fill, no ticks/labels - Remove pointer entirely (showPointer prop, setting, chart option) - Clean centered value with system font Sunburst — smart label management: - Add maxLabelDepth prop to control label visibility by ring depth - Auto-adapt rotation and font size based on segment count per level - Use transparent color (not show:false) for hidden labels so emphasis reveals the full ancestor path on hover - Add DeepHierarchy story (5-level, ~80 cities) and ManyFirstLevel story E2E — use production build locally: - Replace `next dev --webpack` with `next build` + `next start` in global-setup.ts — matches CI behavior, cuts local E2E from 30+ min to ~18 min - Add E2E_SKIP_BUILD env var to skip rebuild when iterating Docs: - Add comprehensive APP_IMPLEMENTATION_GUIDE.md (24 sections) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a Gantt chart feature (component, plugin, transform, settings, stories, tests, and exports), removes the gauge Changes
Sequence Diagram(s)sequenceDiagram
participant Data as Data Source
participant Transform as transformToGanttData
participant Plugin as Gantt Plugin
participant Chart as GanttChart
participant ECharts as ECharts runtime
rect rgba(200,200,255,0.5)
Data->>Transform: raw query result
Transform-->>Plugin: normalized Gantt data
end
rect rgba(200,255,200,0.5)
Plugin->>Chart: props (data, settings, stylingRules)
Chart->>ECharts: setOption(options with custom renderItem)
ECharts-->>Chart: render/update visual
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
component/src/charts/gauge-chart.tsx (1)
1-17:⚠️ Potential issue | 🔴 CriticalWrap GaugeChart with
next/dynamic(..., { ssr: false })in the component library.The chart is exported directly from
component/src/charts/gauge-chart.tsxandcomponent/src/charts/index.tswithout a client-only boundary. ECharts initialization (echarts.use()at line 17) executes at module scope and will evaluate during SSR. While the app layer wraps it at consumption time (e.g.,app/src/plugins/gauge/component.tsx), the component library itself must enforce the client-only boundary. Move thenext/dynamicwrapper to the library's export incomponent/src/charts/gauge-chart.tsxorcomponent/src/charts/index.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/gauge-chart.tsx` around lines 1 - 17, The GaugeChart module initializes echarts at module scope which runs during SSR; wrap the exported GaugeChart component with next/dynamic(..., { ssr: false }) in this library so the echarts.use(...) and GaugeChart rendering only run on the client. Replace/export the GaugeChart (the component defined in gauge-chart.tsx and re-exported from charts/index.ts) as a dynamically imported component via next/dynamic with ssr: false, or move the echarts.use(...) calls into the client-only wrapper so that BaseChart/GaugeChart are only imported/executed on the client.
🤖 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/e2e/global-setup.ts`:
- Around line 267-279: The comment and behavior mismatch: current code only
checks E2E_SKIP_BUILD and will skip building even when .next/BUILD_ID is
missing, causing next start to fail; update the build block in global-setup.ts
to check both that process.env.E2E_SKIP_BUILD is set AND that the BUILD_ID file
exists before skipping (i.e., only skip when BUILD_ID is present), and if
E2E_SKIP_BUILD is set but BUILD_ID is missing, either run the build or throw a
clear error; also remove the unnecessary dynamic await
import("node:child_process") and use the existing static import (replace
execSync import with the already-imported execSync used alongside spawn) so
execSync is available without dynamic import.
In `@app/src/plugins/sunburst/settings.ts`:
- Line 9: The maxLabelDepth schema uses z.coerce.number() which silently turns
blank inputs into 0 and bypasses the default; update the maxLabelDepth schema to
preprocess blank/null/empty-string into undefined (using z.preprocess) before
coercion, then validate with z.number().int().min(1).max(10).default(2) (or
another appropriate max) so empty inputs fall back to the default and values are
constrained; modify the maxLabelDepth line to use
z.preprocess(...)->z.number().int().min(1).max(10).default(2).
In `@component/src/charts/gauge-chart.tsx`:
- Around line 90-102: The progressColor currently ignores thresholdZones; change
progressColor computation so it first uses resolvedColor (from
resolveItemColor(point.value, stylingRules, paramValues)), and if that is
null/undefined then if hasCustomZones is true derive the active progress color
from thresholdZones using the current value (point.value) or the same selection
logic used for trackColor's zones, otherwise fall back to the default "#5470c6";
update references to progressColor, hasCustomZones, thresholdZones and
resolvedColor to implement this selection.
In `@component/src/charts/sunburst-chart.tsx`:
- Around line 26-27: The prop maxLabelDepth currently treats 0 as a real value
due to using nullish coalescing (maxLabelDepth ?? 2), which makes 0 hide labels;
change the normalization to treat 0 as the "auto" sentinel so the component
falls back to the default (e.g., 2) when maxLabelDepth is 0 or undefined. Locate
where maxLabelDepth is normalized (references: the prop name maxLabelDepth and
any internal variable like maxLabelDepthUsed / normalizedMaxLabelDepth or the
logic around lines 26 and 61-65) and update it to: if maxLabelDepth === 0 then
use the auto/default value, otherwise use the provided positive number (or the
existing ?? default). Ensure the comparison logic that decides whether to render
a label uses the normalized value so depth <= normalized behaves correctly.
- Around line 143-157: The series-level label configuration in the Sunburst
chart is ignoring the showLabels prop: update the top-level series label.show
(currently using only !compact) to respect showLabels (i.e., label.show =
showLabels && !compact) and also change the emphasis.block's label.show
(currently hardcoded true) to use showLabels (e.g., emphasis.label.show =
showLabels) so hover/emphasis respects the prop; adjust in the series object
where label, minAngle, and emphasis are defined to be consistent with the
existing levels check that uses showLabels && !compact.
---
Outside diff comments:
In `@component/src/charts/gauge-chart.tsx`:
- Around line 1-17: The GaugeChart module initializes echarts at module scope
which runs during SSR; wrap the exported GaugeChart component with
next/dynamic(..., { ssr: false }) in this library so the echarts.use(...) and
GaugeChart rendering only run on the client. Replace/export the GaugeChart (the
component defined in gauge-chart.tsx and re-exported from charts/index.ts) as a
dynamically imported component via next/dynamic with ssr: false, or move the
echarts.use(...) calls into the client-only wrapper so that BaseChart/GaugeChart
are only imported/executed on the client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff9cae30-5ada-40af-b72b-cb75f82514dd
📒 Files selected for processing (15)
app/e2e/global-setup.tsapp/src/plugins/gauge/component.tsxapp/src/plugins/gauge/settings.tsapp/src/plugins/settings/__tests__/settings-schemas.test.tsapp/src/plugins/sunburst/component.tsxapp/src/plugins/sunburst/settings.tscomponent/src/charts/__tests__/gauge-chart.test.tsxcomponent/src/charts/gauge-chart.tsxcomponent/src/charts/sunburst-chart.tsxcomponent/src/components/composed/chart-options/gauge.tscomponent/src/components/composed/chart-options/sunburst.tscomponent/stories/charts/gauge-chart.stories.tsxcomponent/stories/charts/sunburst-chart.stories.tsxdocs/APP_IMPLEMENTATION_GUIDE.mdscripts/demo/chart-gallery.json
💤 Files with no reviewable changes (4)
- app/src/plugins/gauge/component.tsx
- component/src/components/composed/chart-options/gauge.ts
- app/src/plugins/gauge/settings.ts
- app/src/plugins/settings/tests/settings-schemas.test.ts
Comment on lines
143
to
+157
| label: { | ||
| show: showLabels && !compact, | ||
| show: !compact, | ||
| fontSize: 11, | ||
| }, | ||
| // Hide labels for segments with arc angle below 5 degrees | ||
| // Hide labels on very thin slivers regardless of level settings | ||
| minAngle: 5, | ||
| emphasis: highlightOnHover | ||
| ? { | ||
| focus: "ancestor", | ||
| label: { show: showLabels && !compact }, | ||
| label: { | ||
| show: true, | ||
| fontSize: 12, | ||
| fontWeight: "bold" as const, | ||
| color: "inherit", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the sunburst-chart.tsx file
find . -name "sunburst-chart.tsx" -type fRepository: alfredo1996/neoboard
Length of output: 105
🏁 Script executed:
# Once we have the file, let's examine the full component context
cat -n component/src/charts/sunburst-chart.tsx | head -200Repository: alfredo1996/neoboard
Length of output: 7775
🏁 Script executed:
# Check for showLabels prop definition and its usage throughout the file
rg "showLabels" component/src/charts/sunburst-chart.tsx -B 2 -A 2Repository: alfredo1996/neoboard
Length of output: 1053
🏁 Script executed:
# Check imports to ensure ECharts imports follow the coding guideline
rg "import.*echarts" component/src/charts/sunburst-chart.tsxRepository: alfredo1996/neoboard
Length of output: 339
Respect showLabels={false} in top-level label config and emphasis state.
The levels array correctly checks showLabels && !compact (line 97), but the series-level label.show at line 144 only checks !compact, and emphasis.label.show is hardcoded to true at line 153. When showLabels={false}, labels will still appear on hover, violating the prop's intent.
Suggested fix
label: {
- show: !compact,
+ show: showLabels && !compact,
fontSize: 11,
},
@@
label: {
- show: true,
+ show: showLabels && !compact,
fontSize: 12,
fontWeight: "bold" as const,
color: "inherit",📝 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
| label: { | |
| show: showLabels && !compact, | |
| show: !compact, | |
| fontSize: 11, | |
| }, | |
| // Hide labels for segments with arc angle below 5 degrees | |
| // Hide labels on very thin slivers regardless of level settings | |
| minAngle: 5, | |
| emphasis: highlightOnHover | |
| ? { | |
| focus: "ancestor", | |
| label: { show: showLabels && !compact }, | |
| label: { | |
| show: true, | |
| fontSize: 12, | |
| fontWeight: "bold" as const, | |
| color: "inherit", | |
| }, | |
| label: { | |
| show: showLabels && !compact, | |
| fontSize: 11, | |
| }, | |
| // Hide labels on very thin slivers regardless of level settings | |
| minAngle: 5, | |
| emphasis: highlightOnHover | |
| ? { | |
| focus: "ancestor", | |
| label: { | |
| show: showLabels && !compact, | |
| fontSize: 12, | |
| fontWeight: "bold" as const, | |
| color: "inherit", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/charts/sunburst-chart.tsx` around lines 143 - 157, The
series-level label configuration in the Sunburst chart is ignoring the
showLabels prop: update the top-level series label.show (currently using only
!compact) to respect showLabels (i.e., label.show = showLabels && !compact) and
also change the emphasis.block's label.show (currently hardcoded true) to use
showLabels (e.g., emphasis.label.show = showLabels) so hover/emphasis respects
the prop; adjust in the series object where label, minAngle, and emphasis are
defined to be consistent with the existing levels check that uses showLabels &&
!compact.
- Increase local Playwright workers from 4 to 6 (10-core machine) - Auto-detect cached .next/BUILD_ID — print hint about E2E_SKIP_BUILD - E2E_SKIP_BUILD=1 skips rebuild for instant repeat runs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The transform type selector was rendering both the label and description in the collapsed trigger because Radix SelectValue mirrors all children of SelectItem. Fix by using SelectPrimitive.Item with ItemText wrapping only the label — the description is a sibling outside ItemText, visible in the dropdown but not in the trigger. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New chart type: timeline visualization with horizontal task bars on a time axis. Built with ECharts custom series + renderItem. Features: - Auto-detect task/start/end columns from query results - Color by category via rule-based styling - Progress overlay (optional, 0-1 or 0-100 range) - Today marker (red dashed vertical line) - Data zoom slider for large timelines - Click action support (enrichClickEvent with task, start, end, category) - Responsive with tooltips showing duration Component library: - gantt-chart.tsx — ECharts custom series component - 12 unit tests, 6 Storybook stories (Default, Categories, Progress, LargeDataset, NoTodayLine, EmptyState) - Chart options: showTodayLine, showProgress, showGridLines, barBorderRadius App plugin: - plugins/gantt/ — component, transform, settings, index - Registered as 18th chart type in chart-types.ts - Transform heuristically detects columns with date parsing (ISO, Unix, Date) NeoDash migration: - gantt → gantt mapping in neodash-converter.ts (was falling back to json) Closes #601 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
app/src/components/widget-editor/__tests__/transform-editor.test.tsx (1)
59-70: Mock aligns with the component — LGTM, with one optional add.Partial mock of
Item/ItemText/ItemIndicatorexactly matches whattransform-editor.tsximports viaSelectPrimitive.*, so the tests stay green and will fail loudly if someone reaches for another primitive later. Good minimalism.Optional: none of the existing assertions actually lock in the bug fix (i.e., that the collapsed trigger shows only the label, not
label + description). A single targeted test — e.g., asserting the selected trigger's accessible text equals"Filter"and not"Filter Remove rows matching a condition"— would prevent a regression if someone swaps back to a wrappedSelectItem. Feel free to skip if you'd rather keep the suite lean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/__tests__/transform-editor.test.tsx` around lines 59 - 70, Tests currently mock `@radix-ui/react-select` primitives (Item, ItemText, ItemIndicator) and the suite misses an assertion ensuring the collapsed Select trigger only shows the label (not label + description); add a focused test in transform-editor.test.tsx that renders the transform-editor, selects the relevant item via the mocked SelectPrimitive.Item, and asserts the selected trigger's accessible text equals "Filter" (and does not contain the description like "Remove rows matching a condition") so regressions reverting to wrapped SelectItem (label+description) are caught.app/src/components/widget-editor/transform-editor.tsx (1)
13-13: Fix is correct — trigger now displays onlyt.label.Dropping to
SelectPrimitive.Itemso onlyt.labelis wrapped inItemTextis the right move:@neoboard'sSelectItempresumably wraps all children inItemText, which is why both label and description were bleeding into the collapsed trigger. Description stays visible in the listbox (as a sibling span insideItem) and is still part of the option's accessible name. 👍One optional follow-up to reduce the primitive reach-in: consider adding a
description/subtitleprop to@neoboard/components'sSelectItemso this "label + subtitle" pattern can be reused by other editors (styling-rules-editor,action-rules-editor,field-selector-input) without each of them importing@radix-ui/react-selectdirectly. Not a blocker for this PR.Also applies to: 526-540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/transform-editor.tsx` at line 13, The trigger was showing both label and description because `@neoboard/components`' SelectItem wraps all children in ItemText; switch those option usages to use SelectPrimitive.Item so only t.label is wrapped in ItemText for the trigger. Update occurrences in transform-editor.tsx (the SelectItem imports/usages) to render options with SelectPrimitive.Item (and keep the description as a sibling span inside the Item) so the collapsed trigger shows only t.label; consider mirroring this change for the same pattern at the other occurrences mentioned (lines ~526-540) to keep behavior consistent.component/src/charts/__tests__/gantt-chart.test.tsx (1)
119-134:stylingRulesassertion only checks length.The test is titled "applies styling rule color to matching tasks" but only asserts
seriesData.toHaveLength(3). It won't catch regressions in the rule→color resolution path (e.g., a brokencolumn/operatormatch). Consider inspectingrenderItemoutput or the per-item color metadata attached toseries[0].data[i].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/__tests__/gantt-chart.test.tsx` around lines 119 - 134, The test currently only checks seriesData length and should verify that stylingRules are applied to matching tasks; update the test in gantt-chart.test.tsx to locate the data entries in optionsCall.series[0].data that correspond to tasks with column value "Phase 2" (use the same sampleData field name used by GanttChart) and assert those entries carry the expected color metadata (e.g., an itemStyle.color or color property equal to "#ff0000"); reference GanttChart, stylingRules, mockSetOption, seriesData and renderItem when locating the code paths to validate the per-item color metadata rather than only length.app/src/plugins/gantt/transform.ts (1)
13-18: Return type could be narrowed toGanttDataItem[].Returning
unknownpushes a cast onto every caller (seecomponent.tsxline 32:data as GanttDataItem[]). If the plugintransformsignature allows it, typing this asGanttDataItem[](or at leastArray<Record<string, unknown>>) would give the component.tsx cast something to lean on and keep TS strict mode honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/gantt/transform.ts` around lines 13 - 18, Change the transformToGanttData signature and its returns to a concrete array type instead of unknown: replace the return type unknown with GanttDataItem[] (or Array<Record<string, unknown>> if GanttDataItem isn't available), import or define the GanttDataItem type, and ensure all early returns (e.g., when records are empty or keys.length < 3) return an empty array of that type; update callers (such as component.tsx) to remove unnecessary casts now that transformToGanttData produces the proper typed array.app/src/plugins/__tests__/plugin-options.test.ts (1)
6-24: Addganttto the mock tables for consistency.Two small gaps line up with the new chart type:
OPTION_COUNTS(lines 6–24) has no"gantt"entry, sofakeGetChartOptions("gantt")returns[]. The "every registered chart type has options array" test still passes (empty array is an array), but the gantt plugin ends up with zero options at registration time, which weakens the coverage intent of this file.- The
@neoboard/componentsmock (lines 44–62) doesn't export aGanttChartstub. No current assertion renders it (the plugin'sdynamic()import is lazy), but it's inconsistent with the other chart stubs.♻️ Proposed patch
treemap: 7, + gantt: 8, };TreemapChart: Stub, + GanttChart: Stub, EmptyState: Stub,Also applies to: 44-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/__tests__/plugin-options.test.ts` around lines 6 - 24, The mock OPTION_COUNTS object used by fakeGetChartOptions is missing a "gantt" entry and the `@neoboard/components` test mock doesn't export a GanttChart stub; update OPTION_COUNTS (refer to the constant named OPTION_COUNTS) to include a "gantt": <number> entry (choose a realistic count like 4–8) so fakeGetChartOptions("gantt") returns the expected options array, and add a GanttChart export to the `@neoboard/components` mock (the same module that exports BarChart/LineChart/etc.) so the gantt plugin’s dynamic import has a matching stub (keep the stub shape consistent with other chart stubs).component/stories/charts/gantt-chart.stories.tsx (1)
167-172:Math.random()makesLargeDatasetnon-deterministic.Every Storybook render (and snapshot / Chromatic run) will produce a different end date, which causes visual-regression flakes. A small pseudo-random seeded function or a modulo-based pattern fixes this without losing the "varied durations" feel.
♻️ Deterministic variation
data: Array.from({ length: 30 }, (_, i) => ({ task: `Task ${i + 1}`, start: day(i * 2), - end: day(i * 2 + Math.floor(Math.random() * 8) + 3), + end: day(i * 2 + ((i * 7) % 8) + 3), category: ["Backend", "Frontend", "DevOps", "QA"][i % 4], })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/stories/charts/gantt-chart.stories.tsx` around lines 167 - 172, The story's data array uses Math.random() to compute task end dates which makes the LargeDataset non-deterministic; replace the Math.random() call used in the end calculation (the Array.from(... => ({ task: `Task ${i + 1}`, start: day(...), end: day(...), category: ... }))) with a deterministic variation—either implement a small seeded PRNG and call it for each index or use a deterministic formula like (i * prime) % N to produce the variable duration—so the end value uses day(i * 2 + deterministicOffset + 3) instead of Math.random() to ensure stable Storybook/snapshot outputs while keeping varied durations.component/src/charts/gantt-chart.tsx (1)
253-262: Use ECharts'CustomSeriesRenderItemtype instead ofas never.
renderItem: renderItem as neverbypasses type-checking on the handler. Replace it by importing the proper type from echarts:import type { CustomSeriesRenderItem } from 'echarts';Then type
renderItemdirectly at lines 104–115 instead of casting at line 256. This matches your hand-typed signature and will catch any drift from ECharts' API on upgrades.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/gantt-chart.tsx` around lines 253 - 262, The current series config sidesteps type-checking by using "renderItem: renderItem as never"; instead import and use ECharts' CustomSeriesRenderItem type and type the renderItem function accordingly (the function currently defined around lines 104–115), then remove the "as never" cast in the series object (where renderItem is assigned) so the series entry uses the properly typed renderItem; ensure you add "import type { CustomSeriesRenderItem } from 'echarts'" and annotate the renderItem signature with that type to catch API drift.
🤖 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/plugins/gantt/settings.ts`:
- Line 11: The barBorderRadius schema currently uses
z.coerce.number().default(2) which allows negative numbers; update the
validation for the barBorderRadius symbol to enforce non-negative values by
adding .min(0) (e.g. z.coerce.number().min(0).default(2)); optionally consider
adding an upper bound with .max(...) if you want to cap extreme values to avoid
ECharts rendering issues.
In `@app/src/plugins/gantt/transform.ts`:
- Around line 89-104: parseTime currently uses a fixed 1e12 threshold to decide
seconds vs milliseconds which misclassifies pre-2001 millisecond timestamps;
update parseTime to accept an optional unit hint (e.g., unitHint?: "seconds" |
"milliseconds") and use that when provided, otherwise keep the heuristic but add
a clear comment about its limitations; update the function signature
parseTime(value: unknown, unitHint?: "seconds" | "milliseconds") and apply
unitHint to choose whether to multiply by 1000 (if "seconds") or treat as ms (if
"milliseconds"), leaving the existing fallback heuristic only when unitHint is
absent.
- Around line 76-82: Normalize the progress value as you already do (using
progressKey and local p from row[progressKey]) but then clamp it into the 0..1
range before assigning to item.progress; i.e., after converting percentages (p >
1 ? p/100 : p) apply Math.max(0, Math.min(1, normalized)) so negatives become 0
and overly large percentages (e.g., 150) are capped at 1.
In `@component/src/charts/gantt-chart.tsx`:
- Around line 153-169: The progress overlay uses the same corner radius for all
corners (shape.r = barBorderRadius) which causes the overlay's trailing edge to
appear rounded mid-bar; update the overlay rectangle creation in the render code
that builds the overlay (the block that checks showProgress && progress > 0) to
use a corner-radius array that only rounds the leading/left corners when
progress < 1 (e.g., left-top and left-bottom = barBorderRadius, right corners =
0), and keep full rounding when progress >= 1; additionally clamp progress to
the [0,1] range once when constructing seriesData (the place that currently
feeds Math.round(Number(v[5]) * 100) in the tooltip) so both rendering and
tooltip percent calculations agree.
---
Nitpick comments:
In `@app/src/components/widget-editor/__tests__/transform-editor.test.tsx`:
- Around line 59-70: Tests currently mock `@radix-ui/react-select` primitives
(Item, ItemText, ItemIndicator) and the suite misses an assertion ensuring the
collapsed Select trigger only shows the label (not label + description); add a
focused test in transform-editor.test.tsx that renders the transform-editor,
selects the relevant item via the mocked SelectPrimitive.Item, and asserts the
selected trigger's accessible text equals "Filter" (and does not contain the
description like "Remove rows matching a condition") so regressions reverting to
wrapped SelectItem (label+description) are caught.
In `@app/src/components/widget-editor/transform-editor.tsx`:
- Line 13: The trigger was showing both label and description because
`@neoboard/components`' SelectItem wraps all children in ItemText; switch those
option usages to use SelectPrimitive.Item so only t.label is wrapped in ItemText
for the trigger. Update occurrences in transform-editor.tsx (the SelectItem
imports/usages) to render options with SelectPrimitive.Item (and keep the
description as a sibling span inside the Item) so the collapsed trigger shows
only t.label; consider mirroring this change for the same pattern at the other
occurrences mentioned (lines ~526-540) to keep behavior consistent.
In `@app/src/plugins/__tests__/plugin-options.test.ts`:
- Around line 6-24: The mock OPTION_COUNTS object used by fakeGetChartOptions is
missing a "gantt" entry and the `@neoboard/components` test mock doesn't export a
GanttChart stub; update OPTION_COUNTS (refer to the constant named
OPTION_COUNTS) to include a "gantt": <number> entry (choose a realistic count
like 4–8) so fakeGetChartOptions("gantt") returns the expected options array,
and add a GanttChart export to the `@neoboard/components` mock (the same module
that exports BarChart/LineChart/etc.) so the gantt plugin’s dynamic import has a
matching stub (keep the stub shape consistent with other chart stubs).
In `@app/src/plugins/gantt/transform.ts`:
- Around line 13-18: Change the transformToGanttData signature and its returns
to a concrete array type instead of unknown: replace the return type unknown
with GanttDataItem[] (or Array<Record<string, unknown>> if GanttDataItem isn't
available), import or define the GanttDataItem type, and ensure all early
returns (e.g., when records are empty or keys.length < 3) return an empty array
of that type; update callers (such as component.tsx) to remove unnecessary casts
now that transformToGanttData produces the proper typed array.
In `@component/src/charts/__tests__/gantt-chart.test.tsx`:
- Around line 119-134: The test currently only checks seriesData length and
should verify that stylingRules are applied to matching tasks; update the test
in gantt-chart.test.tsx to locate the data entries in optionsCall.series[0].data
that correspond to tasks with column value "Phase 2" (use the same sampleData
field name used by GanttChart) and assert those entries carry the expected color
metadata (e.g., an itemStyle.color or color property equal to "#ff0000");
reference GanttChart, stylingRules, mockSetOption, seriesData and renderItem
when locating the code paths to validate the per-item color metadata rather than
only length.
In `@component/src/charts/gantt-chart.tsx`:
- Around line 253-262: The current series config sidesteps type-checking by
using "renderItem: renderItem as never"; instead import and use ECharts'
CustomSeriesRenderItem type and type the renderItem function accordingly (the
function currently defined around lines 104–115), then remove the "as never"
cast in the series object (where renderItem is assigned) so the series entry
uses the properly typed renderItem; ensure you add "import type {
CustomSeriesRenderItem } from 'echarts'" and annotate the renderItem signature
with that type to catch API drift.
In `@component/stories/charts/gantt-chart.stories.tsx`:
- Around line 167-172: The story's data array uses Math.random() to compute task
end dates which makes the LargeDataset non-deterministic; replace the
Math.random() call used in the end calculation (the Array.from(... => ({ task:
`Task ${i + 1}`, start: day(...), end: day(...), category: ... }))) with a
deterministic variation—either implement a small seeded PRNG and call it for
each index or use a deterministic formula like (i * prime) % N to produce the
variable duration—so the end value uses day(i * 2 + deterministicOffset + 3)
instead of Math.random() to ensure stable Storybook/snapshot outputs while
keeping varied durations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c681f0ed-0942-4698-aad3-e01d347b2fac
📒 Files selected for processing (21)
app/e2e/global-setup.tsapp/playwright.config.tsapp/src/components/widget-editor/__tests__/transform-editor.test.tsxapp/src/components/widget-editor/transform-editor.tsxapp/src/lib/__tests__/plugin/chart-helpers.test.tsapp/src/lib/dashboard/neodash-converter.tsapp/src/plugins/__tests__/plugin-options.test.tsapp/src/plugins/chart-types.tsapp/src/plugins/gantt/component.tsxapp/src/plugins/gantt/index.tsapp/src/plugins/gantt/settings.tsapp/src/plugins/gantt/transform.tsapp/src/plugins/index.tsapp/src/plugins/transforms/index.tscomponent/src/charts/__tests__/gantt-chart.test.tsxcomponent/src/charts/gantt-chart.tsxcomponent/src/charts/index.tscomponent/src/components/composed/chart-options/gantt.tscomponent/src/components/composed/chart-options/index.tscomponent/stories/charts/gantt-chart.stories.tsxcomponent/vitest.setup.ts
✅ Files skipped from review due to trivial changes (8)
- app/src/lib/tests/plugin/chart-helpers.test.ts
- app/src/lib/dashboard/neodash-converter.ts
- app/src/plugins/chart-types.ts
- component/src/charts/index.ts
- app/src/plugins/gantt/index.ts
- app/src/plugins/transforms/index.ts
- component/vitest.setup.ts
- component/src/components/composed/chart-options/gantt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/e2e/global-setup.ts
| showTodayLine: z.boolean().default(true), | ||
| showProgress: z.boolean().default(false), | ||
| showGridLines: z.boolean().default(true), | ||
| barBorderRadius: z.coerce.number().default(2), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Zod 4 z.coerce.number() accept NaN by default?
💡 Result:
No, Zod 4's z.coerce.number does not accept NaN by default. It coerces the input using JavaScript's Number function, which preserves NaN (Number(NaN) === NaN). The resulting value then passes through z.number validation, which rejects NaN with an "Expected number, received nan" error. To accept NaN, use z.nan or chain .or(z.nan).
Citations:
- 1: https://v4.zod.dev/api?id=numbers
- 2: https://www.mintlify.com/colinhacks/zod/api/utilities/coerce
- 3: Exclude
NaNfromz.number()schema type colinhacks/zod#2189
Correct the borderRadius validation approach.
z.coerce.number().default(2) will accept any valid number including negatives and extreme values. However, Zod 4's z.coerce.number() actually rejects NaN by default—it's not a concern. That said, reasonable bounds prevent unexpected ECharts rendering issues. Consider adding .min(0) to disallow negative radius:
♻️ Proposed tightening
- barBorderRadius: z.coerce.number().default(2),
+ barBorderRadius: z.coerce.number().min(0).default(2),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/gantt/settings.ts` at line 11, The barBorderRadius schema
currently uses z.coerce.number().default(2) which allows negative numbers;
update the validation for the barBorderRadius symbol to enforce non-negative
values by adding .min(0) (e.g. z.coerce.number().min(0).default(2)); optionally
consider adding an upper bound with .max(...) if you want to cap extreme values
to avoid ECharts rendering issues.
Comment on lines
+89
to
+104
| function parseTime(value: unknown): number | null { | ||
| if (value == null) return null; | ||
| if (value instanceof Date) return value.getTime(); | ||
| if (typeof value === "number") { | ||
| // Heuristic: if < 1e12, assume seconds; otherwise milliseconds | ||
| return value < 1e12 ? value * 1000 : value; | ||
| } | ||
| if (typeof value === "string") { | ||
| const parsed = Date.parse(value); | ||
| if (!Number.isNaN(parsed)) return parsed; | ||
| // Try as numeric string | ||
| const num = Number(value); | ||
| if (!Number.isNaN(num)) return num < 1e12 ? num * 1000 : num; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
parseTime seconds-vs-ms heuristic will misclassify pre-2001 ms timestamps.
The < 1e12 threshold (Sep 2001) means any millisecond timestamp older than that gets multiplied by 1000 and lands in the far future. Almost certainly irrelevant for Gantt data in practice, but worth a comment or a different signal (e.g., column metadata / explicit unit) if you expect historical schedules. Leaving as-is is acceptable; just want it on the record.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/plugins/gantt/transform.ts` around lines 89 - 104, parseTime
currently uses a fixed 1e12 threshold to decide seconds vs milliseconds which
misclassifies pre-2001 millisecond timestamps; update parseTime to accept an
optional unit hint (e.g., unitHint?: "seconds" | "milliseconds") and use that
when provided, otherwise keep the heuristic but add a clear comment about its
limitations; update the function signature parseTime(value: unknown, unitHint?:
"seconds" | "milliseconds") and apply unitHint to choose whether to multiply by
1000 (if "seconds") or treat as ms (if "milliseconds"), leaving the existing
fallback heuristic only when unitHint is absent.
Comment on lines
+153
to
+169
| // Progress overlay | ||
| if (showProgress && progress > 0) { | ||
| const progressWidth = width * Math.min(progress, 1); | ||
| (group.children as unknown[]).push({ | ||
| type: "rect", | ||
| shape: { | ||
| x, | ||
| y, | ||
| width: progressWidth, | ||
| height: barHeight, | ||
| r: barBorderRadius, | ||
| }, | ||
| style: { | ||
| fill: "rgba(255, 255, 255, 0.3)", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Progress overlay rounds its trailing edge mid-bar.
The overlay rect reuses r: barBorderRadius on all four corners. When progress < 1, the overlay is narrower than the full bar, so its right-hand corners are rounded in the middle of a still-straight bar edge — the overlay visually "pulls away" from the underlying bar.
♻️ Flatten the trailing corners (or only round when progress ≈ 1)
- (group.children as unknown[]).push({
- type: "rect",
- shape: {
- x,
- y,
- width: progressWidth,
- height: barHeight,
- r: barBorderRadius,
- },
+ const full = progress >= 1;
+ (group.children as unknown[]).push({
+ type: "rect",
+ shape: {
+ x,
+ y,
+ width: progressWidth,
+ height: barHeight,
+ // Only round the right side when the overlay spans the whole bar.
+ r: full
+ ? barBorderRadius
+ : [barBorderRadius, 0, 0, barBorderRadius],
+ },Also worth noting: Math.round(Number(v[5]) * 100) in the tooltip (Line 209) will happily print 150% if a caller passes progress > 1. Consider clamping progress to [0, 1] once when building seriesData so both the renderer and the tooltip agree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/charts/gantt-chart.tsx` around lines 153 - 169, The progress
overlay uses the same corner radius for all corners (shape.r = barBorderRadius)
which causes the overlay's trailing edge to appear rounded mid-bar; update the
overlay rectangle creation in the render code that builds the overlay (the block
that checks showProgress && progress > 0) to use a corner-radius array that only
rounds the leading/left corners when progress < 1 (e.g., left-top and
left-bottom = barBorderRadius, right corners = 0), and keep full rounding when
progress >= 1; additionally clamp progress to the [0,1] range once when
constructing seriesData (the place that currently feeds Math.round(Number(v[5])
* 100) in the tooltip) so both rendering and tooltip percent calculations agree.
- Add page 18 (Gantt) to chart-gallery.json demo showcase - Add E2E creation flow test for Gantt in new-charts.spec.ts - Add vertical Y-axis dataZoom for task lists with 15+ items — shows a scrollbar slider on the right edge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rules with a `column` field now match against that exact data property instead of only checking category/task. Rules without `column` fall back to category then task name for backward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Converter improvements: - Map graph3d/3d-graph → graph (was falling back to json) - Map circle_packing → sunburst (same hierarchical data) - Map choropleth/areamap → map (best-effort, point markers) - Preserve report.title → widget settings.title (was dropped) Test coverage (21 → 50 tests): - All new type mappings (gantt, graph3d, circle_packing, choropleth, areamap) - Widget title preservation (present, multiple, empty) - Multiple parameter conversion in single query - Non-neodash parameters left unchanged - Degraded type conversions documented in test names E2E fixture: - Added gantt and graph3d widgets to neodash-sample.json (4 → 6 widgets) - Updated portability spec to verify 6 widgets render after import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
global-setup.ts: - Guard E2E_SKIP_BUILD: throw if set without .next/BUILD_ID - Use static execSync import instead of dynamic await import() gauge-chart.tsx: - Use threshold zone color for progress arc when custom zones are set sunburst-chart.tsx: - Treat maxLabelDepth=0 as auto (default 2) instead of "show nothing" gantt/transform.ts: - Clamp progress to [0, 1] after normalization - Document parseTime seconds-vs-ms heuristic limitation gantt-chart.tsx: - Fix progress overlay corner radius: only round right side when full - Use "inherit" for today-line label color (adapts to dark mode) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ChartType union includes "gantt" but the chartTypeIcons map was missing the entry, causing a TypeScript error on build. Added the GanttChart icon from lucide-react. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
component/src/charts/gantt-chart.tsx (1)
113-123:⚠️ Potential issue | 🟡 MinorClamp
progresswhen buildingseriesData, not only at render time.
item.progress ?? 0is stored raw, while only the overlay width clamps viaMath.min(progress, 1)(Line 177). The tooltip at Line 235 reusesv[5]verbatim, so a caller passingprogress = 1.5will render correctly-sized bars but a cheerfulProgress: 150%in the tooltip. Clamp once here and both sites stay in sync.♻️ Proposed fix
const seriesData = data.map((item, i) => ({ value: [ i, item.start, item.end, item.end - item.start, item.category ?? "", - item.progress ?? 0, + Math.max(0, Math.min(item.progress ?? 0, 1)), ], itemStyle: resolvedColors[i] ? { color: resolvedColors[i] } : undefined, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/gantt-chart.tsx` around lines 113 - 123, When building seriesData in the data.map callback, clamp the progress value before storing it (replace item.progress ?? 0 with a clamped value like Math.max(0, Math.min(item.progress ?? 0, 1))) so the stored v[5] used by both the rendered overlay and the tooltip stays consistent; update the seriesData construction (the map creating value: [..., item.progress ?? 0, ...]) to use the clamped progress and leave resolvedColors/itemStyle unchanged.
🧹 Nitpick comments (2)
component/src/charts/gauge-chart.tsx (1)
109-111: Prefer a typed ECharts color overas never.The double
as nevercast bypasses the type system instead of satisfying it. ECharts'lineStyle.coloracceptsstring | [number, string][], so a narrower type (or a shared alias) is cleaner and safer against future refactors.♻️ Suggested tweak
- const trackColor = hasCustomZones - ? (thresholdZones as never) - : ([[1, "rgba(140, 140, 140, 0.15)"]] as never); + const trackColor: [number, string][] = hasCustomZones + ? thresholdZones + : [[1, "rgba(140, 140, 140, 0.15)"]];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@component/src/charts/gauge-chart.tsx` around lines 109 - 111, The code uses "as never" to silence the type system for trackColor; instead declare/use a proper ECharts color type (e.g., type EChartsColor = string | [number, string][]) and apply it to trackColor and/or thresholdZones so the union expression matches ECharts' lineStyle.color; update the expression in gauge-chart.tsx that sets trackColor (referencing trackColor, hasCustomZones, thresholdZones) to use this EChartsColor type or cast to EChartsColor rather than using "as never", or annotate the original thresholdZones declaration with EChartsColor so no unsafe cast is needed.app/src/plugins/gantt/transform.ts (1)
13-13: Return typeunknownleaks heuristics to every caller.
transformToGanttDatahas well-defined shape ({ task, start, end, category?, progress? }[]). Returningunknownforces consumers (component.tsx, transforms barrel) to re-assert/cast. Consider exposing aGanttDatumtype so the Gantt component gets compile-time safety onstart/end/progress.♻️ Proposed tweak
+export interface GanttDatum { + task: string; + start: number; + end: number; + category?: string; + progress?: number; +} + -export function transformToGanttData(data: unknown): unknown { +export function transformToGanttData(data: unknown): GanttDatum[] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/plugins/gantt/transform.ts` at line 13, The function transformToGanttData currently returns unknown and should instead return a well-typed array; define a GanttDatum type/interface with required fields { task: string; start: Date | string; end: Date | string; category?: string; progress?: number } and change transformToGanttData signature to return GanttDatum[]; update any callers (component.tsx and the transforms barrel) to use GanttDatum instead of casting, and ensure any internal parsing/conversion within transformToGanttData produces values matching those types (e.g., convert start/end to Date or a consistent string format).
🤖 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/e2e/global-setup.ts`:
- Around line 272-297: The code currently treats process.env.E2E_SKIP_BUILD as a
boolean which makes values like "0" or "false" still count as truthy and skip
the build; update the checks around process.env.E2E_SKIP_BUILD (the block that
decides whether to skip or run execSync("npx next build", { cwd: appDir, stdio:
"inherit", env: serverEnv })) to explicitly parse/normalize the env var (e.g.
treat "0", "false", "no" as falsy and "1", "true", "yes" as truthy) before
branching; use the normalized boolean in both the initial hasCachedBuild guard
and the later if/else that logs skipping vs building so a user setting
E2E_SKIP_BUILD=0 will not skip the rebuild.
In `@component/src/charts/gantt-chart.tsx`:
- Around line 224-237: The tooltip formatter can call echarts.format.encodeHTML
with an undefined name when v[0] is out of range; in the formatter function
(formatter: (params: unknown) => { ... }) validate that params.value is an array
and that taskIndex = Number(v[0]) is an integer inside the bounds of taskNames,
then derive name = taskNames[taskIndex] ?? 'Unknown task' (or empty string)
before calling echarts.format.encodeHTML; also guard accesses to v[1..5] (coerce
to numbers or provide safe defaults) so formatDuration(v[3]) and progress
calculation never receive undefined.
In `@component/src/charts/gauge-chart.tsx`:
- Around line 102-105: The lookup for thresholdColor assumes thresholdZones are
ordered but parseGaugeThresholdZones preserves user order; to fix, ensure the
zones used for color selection are sorted ascending by their stop value before
performing the find — e.g., sort the thresholdZones array (the one used in
gauge-chart.tsx where thresholdColor is computed) by the numeric stop ([stop])
so thresholdZones.find(([stop]) => normalizedValue <= stop) yields the correct
zone; alternatively, enforce sorting inside parseGaugeThresholdZones so any
consumer (including thresholdColor computation) receives zones in ascending
order.
---
Duplicate comments:
In `@component/src/charts/gantt-chart.tsx`:
- Around line 113-123: When building seriesData in the data.map callback, clamp
the progress value before storing it (replace item.progress ?? 0 with a clamped
value like Math.max(0, Math.min(item.progress ?? 0, 1))) so the stored v[5] used
by both the rendered overlay and the tooltip stays consistent; update the
seriesData construction (the map creating value: [..., item.progress ?? 0, ...])
to use the clamped progress and leave resolvedColors/itemStyle unchanged.
---
Nitpick comments:
In `@app/src/plugins/gantt/transform.ts`:
- Line 13: The function transformToGanttData currently returns unknown and
should instead return a well-typed array; define a GanttDatum type/interface
with required fields { task: string; start: Date | string; end: Date | string;
category?: string; progress?: number } and change transformToGanttData signature
to return GanttDatum[]; update any callers (component.tsx and the transforms
barrel) to use GanttDatum instead of casting, and ensure any internal
parsing/conversion within transformToGanttData produces values matching those
types (e.g., convert start/end to Date or a consistent string format).
In `@component/src/charts/gauge-chart.tsx`:
- Around line 109-111: The code uses "as never" to silence the type system for
trackColor; instead declare/use a proper ECharts color type (e.g., type
EChartsColor = string | [number, string][]) and apply it to trackColor and/or
thresholdZones so the union expression matches ECharts' lineStyle.color; update
the expression in gauge-chart.tsx that sets trackColor (referencing trackColor,
hasCustomZones, thresholdZones) to use this EChartsColor type or cast to
EChartsColor rather than using "as never", or annotate the original
thresholdZones declaration with EChartsColor so no unsafe cast is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf0e6cc0-f63a-4e5a-8614-a84f991b5093
📒 Files selected for processing (12)
app/e2e/dashboard-portability.spec.tsapp/e2e/fixtures/imports/neodash-sample.jsonapp/e2e/global-setup.tsapp/e2e/new-charts.spec.tsapp/src/components/widget-editor/chart-type-selector.tsxapp/src/lib/__tests__/dashboard/neodash-converter.test.tsapp/src/lib/dashboard/neodash-converter.tsapp/src/plugins/gantt/transform.tscomponent/src/charts/gantt-chart.tsxcomponent/src/charts/gauge-chart.tsxcomponent/src/charts/sunburst-chart.tsxscripts/demo/chart-gallery.json
✅ Files skipped from review due to trivial changes (1)
- app/src/components/widget-editor/chart-type-selector.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/lib/dashboard/neodash-converter.ts
- scripts/demo/chart-gallery.json
Comment on lines
+272
to
+297
| if (process.env.E2E_SKIP_BUILD && !hasCachedBuild) { | ||
| throw new Error( | ||
| "E2E_SKIP_BUILD is set but no prior build found at .next/BUILD_ID. " + | ||
| "Run `npx next build` once or unset E2E_SKIP_BUILD.", | ||
| ); | ||
| } | ||
|
|
||
| if (process.env.E2E_SKIP_BUILD) { | ||
| console.log( | ||
| "⏩ Skipping build (E2E_SKIP_BUILD set, reusing existing .next)", | ||
| ); | ||
| } else { | ||
| if (hasCachedBuild) { | ||
| console.log( | ||
| "⏳ Rebuilding Next.js (production)... (set E2E_SKIP_BUILD=1 to reuse previous build)", | ||
| ); | ||
| } else { | ||
| console.log("⏳ Building Next.js (production)..."); | ||
| } | ||
| execSync("npx next build", { | ||
| cwd: appDir, | ||
| stdio: "inherit", | ||
| env: serverEnv, | ||
| }); | ||
| console.log("✅ Next.js build complete"); | ||
| } |
There was a problem hiding this comment.
E2E_SKIP_BUILD=0 is truthy and will still skip the build.
process.env.E2E_SKIP_BUILD is a string, so E2E_SKIP_BUILD=0 / false / "" (unset via export then overridden) all evaluate truthy (except ""). A contributor who flips this to 0 expecting "off" will unintentionally skip the rebuild and quietly run stale bundles in E2E. Consider parsing it explicitly.
♻️ Proposed tweak
- if (process.env.E2E_SKIP_BUILD && !hasCachedBuild) {
+ const skipBuild = /^(1|true|yes)$/i.test(process.env.E2E_SKIP_BUILD ?? "");
+
+ if (skipBuild && !hasCachedBuild) {
throw new Error(
"E2E_SKIP_BUILD is set but no prior build found at .next/BUILD_ID. " +
"Run `npx next build` once or unset E2E_SKIP_BUILD.",
);
}
- if (process.env.E2E_SKIP_BUILD) {
+ if (skipBuild) {
console.log(
"⏩ Skipping build (E2E_SKIP_BUILD set, reusing existing .next)",
);
} else {📝 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 (process.env.E2E_SKIP_BUILD && !hasCachedBuild) { | |
| throw new Error( | |
| "E2E_SKIP_BUILD is set but no prior build found at .next/BUILD_ID. " + | |
| "Run `npx next build` once or unset E2E_SKIP_BUILD.", | |
| ); | |
| } | |
| if (process.env.E2E_SKIP_BUILD) { | |
| console.log( | |
| "⏩ Skipping build (E2E_SKIP_BUILD set, reusing existing .next)", | |
| ); | |
| } else { | |
| if (hasCachedBuild) { | |
| console.log( | |
| "⏳ Rebuilding Next.js (production)... (set E2E_SKIP_BUILD=1 to reuse previous build)", | |
| ); | |
| } else { | |
| console.log("⏳ Building Next.js (production)..."); | |
| } | |
| execSync("npx next build", { | |
| cwd: appDir, | |
| stdio: "inherit", | |
| env: serverEnv, | |
| }); | |
| console.log("✅ Next.js build complete"); | |
| } | |
| const skipBuild = /^(1|true|yes)$/i.test(process.env.E2E_SKIP_BUILD ?? ""); | |
| if (skipBuild && !hasCachedBuild) { | |
| throw new Error( | |
| "E2E_SKIP_BUILD is set but no prior build found at .next/BUILD_ID. " + | |
| "Run `npx next build` once or unset E2E_SKIP_BUILD.", | |
| ); | |
| } | |
| if (skipBuild) { | |
| console.log( | |
| "⏩ Skipping build (E2E_SKIP_BUILD set, reusing existing .next)", | |
| ); | |
| } else { | |
| if (hasCachedBuild) { | |
| console.log( | |
| "⏳ Rebuilding Next.js (production)... (set E2E_SKIP_BUILD=1 to reuse previous build)", | |
| ); | |
| } else { | |
| console.log("⏳ Building Next.js (production)..."); | |
| } | |
| execSync("npx next build", { | |
| cwd: appDir, | |
| stdio: "inherit", | |
| env: serverEnv, | |
| }); | |
| console.log("✅ Next.js build complete"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/e2e/global-setup.ts` around lines 272 - 297, The code currently treats
process.env.E2E_SKIP_BUILD as a boolean which makes values like "0" or "false"
still count as truthy and skip the build; update the checks around
process.env.E2E_SKIP_BUILD (the block that decides whether to skip or run
execSync("npx next build", { cwd: appDir, stdio: "inherit", env: serverEnv }))
to explicitly parse/normalize the env var (e.g. treat "0", "false", "no" as
falsy and "1", "true", "yes" as truthy) before branching; use the normalized
boolean in both the initial hasCachedBuild guard and the later if/else that logs
skipping vs building so a user setting E2E_SKIP_BUILD=0 will not skip the
rebuild.
Comment on lines
+224
to
+237
| formatter: (params: unknown) => { | ||
| const p = params as { value: number[] }; | ||
| const v = p.value; | ||
| const name = taskNames[v[0]]; | ||
| const start = new Date(v[1]).toLocaleDateString(); | ||
| const end = new Date(v[2]).toLocaleDateString(); | ||
| const duration = formatDuration(v[3]); | ||
| const category = v[4] | ||
| ? `<br/>Category: ${echarts.format.encodeHTML(String(v[4]))}` | ||
| : ""; | ||
| const progress = | ||
| v[5] > 0 ? `<br/>Progress: ${Math.round(Number(v[5]) * 100)}%` : ""; | ||
| return `<strong>${echarts.format.encodeHTML(name)}</strong><br/>${start} → ${end} (${duration})${category}${progress}`; | ||
| }, |
There was a problem hiding this comment.
Guard against out-of-range taskIndex in the tooltip.
If v[0] ever falls outside taskNames (e.g., stale data frame), name is undefined and echarts.format.encodeHTML(name) is called on a non-string. Cheap fallback:
♻️ Proposed fix
- const name = taskNames[v[0]];
+ const name = taskNames[v[0]] ?? "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/charts/gantt-chart.tsx` around lines 224 - 237, The tooltip
formatter can call echarts.format.encodeHTML with an undefined name when v[0] is
out of range; in the formatter function (formatter: (params: unknown) => { ...
}) validate that params.value is an array and that taskIndex = Number(v[0]) is
an integer inside the bounds of taskNames, then derive name =
taskNames[taskIndex] ?? 'Unknown task' (or empty string) before calling
echarts.format.encodeHTML; also guard accesses to v[1..5] (coerce to numbers or
provide safe defaults) so formatDuration(v[3]) and progress calculation never
receive undefined.
Comment on lines
+102
to
+105
| const thresholdColor = | ||
| hasCustomZones && normalizedValue !== undefined | ||
| ? thresholdZones.find(([stop]) => normalizedValue <= stop)?.[1] | ||
| : undefined; |
There was a problem hiding this comment.
Threshold-zone lookup silently assumes ascending order.
parseGaugeThresholdZones returns zones in the same order the user authored them (no sort), so thresholdZones.find(([stop]) => normalizedValue <= stop) will pick the wrong color if a user configures zones out of order (e.g. [{value:90,color:"red"},{value:50,color:"yellow"}] → everything ≤ 90 resolves to red). ECharts' own axisLine rendering has the same sensitivity, but there the mis-color is at least consistent with the progress arc; here the two can disagree visually only in edge cases — worth either sorting once, or documenting the contract.
🛠️ Suggested hardening
const thresholdZones = parseGaugeThresholdZones(
thresholdZonesJson,
min,
max,
- ) as [number, string][];
+ ).slice().sort((a, b) => a[0] - b[0]) as [number, string][];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/charts/gauge-chart.tsx` around lines 102 - 105, The lookup for
thresholdColor assumes thresholdZones are ordered but parseGaugeThresholdZones
preserves user order; to fix, ensure the zones used for color selection are
sorted ascending by their stop value before performing the find — e.g., sort the
thresholdZones array (the one used in gauge-chart.tsx where thresholdColor is
computed) by the numeric stop ([stop]) so thresholdZones.find(([stop]) =>
normalizedValue <= stop) yields the correct zone; alternatively, enforce sorting
inside parseGaugeThresholdZones so any consumer (including thresholdColor
computation) receives zones in ascending order.
Fixes TS6133 "'params' is declared but its value is never read" in CI type-check step. The params argument is required by ECharts renderItem signature but unused in our implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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
maxLabelDepthprop for smart label management — auto-hides labels on crowded rings, reveals full ancestor path on hovernext dev --webpackwithnext build+next startin global-setup — cuts local E2E runtime from 30+ min to ~18 minAPP_IMPLEMENTATION_GUIDE.mdcovering all 24 sections of the app/ package architectureTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
Chores
Documentation / Stories
Tests