Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ jobs:

- name: Build connection package
run: npm -w connection run build
continue-on-error: true
Comment on lines 55 to +57

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Failing connection builds are being silently ignored in required CI gates.

continue-on-error: true at Line 57, Line 117, and Line 222 makes npm -w connection run build non-blocking in typecheck, unit-tests, and e2e. That can hide real regressions and let broken connection code pass merge gates.

Suggested fix
       - name: Build connection package
         run: npm -w connection run build
-        continue-on-error: true

Apply this removal in all three jobs.

Based on learnings: Run npm run build before committing to catch type errors.

Also applies to: 115-117, 220-222

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 55 - 57, Remove the
"continue-on-error: true" flag from the steps that run "npm -w connection run
build" so failing builds are not ignored; specifically edit the three steps
named "Build connection package" in the typecheck, unit-tests, and e2e jobs to
delete the continue-on-error line, leaving the run: npm -w connection run build
command intact so CI will fail on connection build errors.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional — the connection package has 16 upstream neo4j-driver-core type errors that we can't fix (driver type mismatch). Build emits JS despite errors via noEmitOnError: false. The continue-on-error prevents GitHub Actions from failing the step on error annotations. Tracked in #442 for proper resolution when neo4j-driver-core updates.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alfredo1996, understood — that's a clear and reasonable workaround. With noEmitOnError: false letting the JS emit through, and #442 tracking the upstream fix, continue-on-error: true is the right guard here. I'll note this for future reviews.


✏️ Learnings added
Learnt from: alfredo1996
Repo: alfredo1996/neoboard PR: 471
File: .github/workflows/ci.yml:55-57
Timestamp: 2026-04-09T12:51:30.347Z
Learning: In `alfredo1996/neoboard`, the `continue-on-error: true` flag on the "Build connection package" step (`npm -w connection run build`) in `.github/workflows/ci.yml` (jobs: `typecheck`, `unit-tests`, `e2e`) is intentional. The `connection` package has ~16 unfixable upstream type errors from `neo4j-driver-core` (driver type mismatch); `noEmitOnError: false` in `connection/tsconfig.json` still emits JS. `continue-on-error: true` prevents GHA from marking the step/job as failed due to `tsc` error exit codes. Tracked for proper resolution in issue `#442` when `neo4j-driver-core` updates its types.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: alfredo1996/neoboard PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:09:06.308Z
Learning: Run `npm run build` before committing to catch type errors.

Learnt from: CR
Repo: alfredo1996/neoboard PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:09:06.308Z
Learning: Applies to connection/**/*.{ts,tsx} : Enforce timeouts at driver level (AbortSignal for PostgreSQL, native for Neo4j). Default 30s.

Learnt from: CR
Repo: alfredo1996/neoboard PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:09:06.308Z
Learning: Applies to connection/**/*.{ts,tsx} : Do NOT import from `app/` or `component/` packages in `connection/` package files. NO UI, NO React in connection/ package.

Learnt from: CR
Repo: alfredo1996/neoboard PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T19:09:06.308Z
Learning: PRs need labels: type + package + area. Link issue via `Closes `#N``. Correct milestone/labels after finishing.


- name: Type-check component
run: npm -w component exec tsc -- --noEmit
Expand Down Expand Up @@ -113,6 +114,7 @@ jobs:

- name: Build connection package
run: npm -w connection run build
continue-on-error: true

- name: Run all tests in parallel
run: |
Expand Down Expand Up @@ -217,6 +219,7 @@ jobs:

- name: Build connection package
run: npm -w connection run build
continue-on-error: true

- name: Build Next.js
working-directory: app
Expand Down
4 changes: 3 additions & 1 deletion app/e2e/widget-states.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,9 @@ test.describe("Widget editor", () => {
await actionsBtn.click();

// Should show Edit and Remove menu items
await expect(page.getByRole("menuitem", { name: "Edit" })).toBeVisible();
await expect(
page.getByRole("menuitem", { name: "Edit Widget" }),
).toBeVisible();
await expect(
page.getByRole("menuitem", { name: "Remove" }),
).toBeVisible();
Expand Down
2 changes: 1 addition & 1 deletion app/e2e/widgets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ test.describe("Widget edit – query cache invalidation", () => {
);

await actionsBtn.click();
await page.getByRole("menuitem", { name: "Edit" }).click();
await page.getByRole("menuitem", { name: "Edit Widget" }).click();

const editDialog = page.getByRole("dialog", { name: "Edit Widget" });
await expect(editDialog).toBeVisible({ timeout: 10_000 });
Expand Down
2 changes: 1 addition & 1 deletion app/src/components/dashboard-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export function DashboardContainer({
if (!editable) return actions.length > 0 ? actions : undefined;
if (onEditWidget) {
actions.push({
label: "Edit",
label: "Edit Widget",
onClick: () => onEditWidget(widget),
});
}
Expand Down
4 changes: 2 additions & 2 deletions app/src/lib/__tests__/dashboard/dashboard-import.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ describe("widget settings validation — misplaced chartOptions", () => {
chartType: "pie",
connectionId: "c1",
query: "q",
settings: { title: "Bad", colorPalette: "neon" },
settings: { title: "Bad", colorPalette: "tableau" },
},
],
gridLayout: [{ i: "w1", x: 0, y: 0, w: 6, h: 4 }],
Expand Down Expand Up @@ -382,7 +382,7 @@ describe("widget settings validation — misplaced chartOptions", () => {
query: "q",
settings: {
title: "Good",
chartOptions: { colorPalette: "neon" },
chartOptions: { colorPalette: "tableau" },
},
},
],
Expand Down
13 changes: 6 additions & 7 deletions component/src/charts/__tests__/base-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,27 +183,26 @@ describe("BaseChart", () => {
);
});

it("overrides colors with warm-sunset palette when colorPalette is set", () => {
it("overrides colors with tableau palette when colorPalette is set", () => {
render(
<BaseChart
options={{ title: { text: "Test" } }}
colorPalette="warm-sunset"
colorPalette="tableau"
/>,
);
expect(mockSetOption).toHaveBeenCalledWith(
expect.objectContaining({
// warm-sunset first color is tomato red
color: expect.arrayContaining(["hsl(14, 90%, 55%)"]),
color: expect.arrayContaining(["#4e79a7"]),
}),
{ notMerge: true },
);
});

it("overrides colors with neon palette when colorPalette is set", () => {
render(<BaseChart options={{}} colorPalette="neon" />);
it("overrides colors with observable palette when colorPalette is set", () => {
render(<BaseChart options={{}} colorPalette="observable" />);
expect(mockSetOption).toHaveBeenCalledWith(
expect.objectContaining({
color: expect.arrayContaining(["hsl(320, 100%, 60%)"]),
color: expect.arrayContaining(["#4269d0"]),
}),
{ notMerge: true },
);
Expand Down
59 changes: 38 additions & 21 deletions component/src/charts/__tests__/gauge-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,40 +67,34 @@ describe("GaugeChart", () => {
});

// --- axisTick distance bug fix ---
it("sets axisTick.distance to -15 in non-compact mode (bug fix: was -compact ? 0 : 15)", () => {
// In non-compact mode (container >= 200px), axisTick.distance must be -15 (inward).
// The old code used `-compact ? 0 : 15` which always evaluated to 15 due to unary minus on boolean.
it("sets axisTick.distance to -20 in non-compact mode", () => {
render(<GaugeChart data={sampleData} />);
const optionsCall = mockSetOption.mock.calls[0][0];
const series = optionsCall.series[0];
// Non-compact: axisTick is shown and distance should be -15 (negative = inward from arc)
expect(series.axisTick.show).toBe(true);
expect(series.axisTick.distance).toBe(-15);
expect(series.axisTick.distance).toBe(-20);
});

// --- splitLine distance fix ---
it("sets splitLine.distance to -25 in non-compact mode to push lines further inward", () => {
it("sets splitLine.distance to -20 in non-compact mode", () => {
render(<GaugeChart data={sampleData} />);
const optionsCall = mockSetOption.mock.calls[0][0];
const series = optionsCall.series[0];
expect(series.splitLine.show).toBe(true);
expect(series.splitLine.distance).toBe(-25);
expect(series.splitLine.distance).toBe(-20);
});

// --- axisLabel distance fix ---
it("sets axisLabel.distance to 35 in non-compact mode for more space between labels and arc", () => {
it("sets axisLabel.distance to 30 in non-compact mode", () => {
render(<GaugeChart data={sampleData} />);
const optionsCall = mockSetOption.mock.calls[0][0];
const series = optionsCall.series[0];
expect(series.axisLabel.show).toBe(true);
expect(series.axisLabel.distance).toBe(35);
expect(series.axisLabel.distance).toBe(30);
});

// --- axisLabel fontSize ---
it("sets axisLabel.fontSize to 12 to reduce label size", () => {
it("sets axisLabel.fontSize to 11", () => {
render(<GaugeChart data={sampleData} />);
const optionsCall = mockSetOption.mock.calls[0][0];
expect(optionsCall.series[0].axisLabel.fontSize).toBe(12);
expect(optionsCall.series[0].axisLabel.fontSize).toBe(11);
});

// --- axisTick splitNumber ---
Expand All @@ -123,25 +117,47 @@ describe("GaugeChart", () => {
// --- styling rules ---

it("applies styling rule color to gauge item when value matches rule", () => {
const stylingRules = [{ id: "r1", operator: ">" as const, value: 50, color: "#ff0000" }];
render(<GaugeChart data={[{ value: 75, name: "Score" }]} stylingRules={stylingRules} />);
const stylingRules = [
{ id: "r1", operator: ">" as const, value: 50, color: "#ff0000" },
];
render(
<GaugeChart
data={[{ value: 75, name: "Score" }]}
stylingRules={stylingRules}
/>,
);
const optionsCall = mockSetOption.mock.calls[0][0];
const gaugeData = optionsCall.series[0].data[0];
expect(gaugeData.itemStyle?.color).toBe("#ff0000");
});

it("does not apply color when value does not match any styling rule", () => {
const stylingRules = [{ id: "r1", operator: ">" as const, value: 90, color: "#ff0000" }];
render(<GaugeChart data={[{ value: 75, name: "Score" }]} stylingRules={stylingRules} />);
const stylingRules = [
{ id: "r1", operator: ">" as const, value: 90, color: "#ff0000" },
];
render(
<GaugeChart
data={[{ value: 75, name: "Score" }]}
stylingRules={stylingRules}
/>,
);
const optionsCall = mockSetOption.mock.calls[0][0];
const gaugeData = optionsCall.series[0].data[0];
expect(gaugeData.itemStyle).toBeUndefined();
});

it("accepts paramValues prop without error", () => {
const stylingRules = [{ id: "r1", operator: ">=" as const, value: 50, color: "#00ff00" }];
const stylingRules = [
{ id: "r1", operator: ">=" as const, value: 50, color: "#00ff00" },
];
const paramValues = { threshold: 50 };
render(<GaugeChart data={sampleData} stylingRules={stylingRules} paramValues={paramValues} />);
render(
<GaugeChart
data={sampleData}
stylingRules={stylingRules}
paramValues={paramValues}
/>,
);
expect(screen.getByTestId("base-chart")).toBeInTheDocument();
});

Expand Down Expand Up @@ -170,7 +186,8 @@ describe("GaugeChart", () => {
expect(series.axisTick.show).toBe(false);
expect(series.splitLine.show).toBe(false);
expect(series.axisLabel.show).toBe(false);
expect(series.detail.show).toBe(false);
// Detail stays visible in compact mode (smaller font), title hides
expect(series.detail.show).toBe(true);
expect(series.title.show).toBe(false);
});
});
34 changes: 19 additions & 15 deletions component/src/charts/__tests__/palettes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ describe("COLOR_PALETTES", () => {

it("every palette has exactly 10 colors", () => {
for (const [id, palette] of Object.entries(COLOR_PALETTES)) {
expect(palette.colors, `palette "${id}" should have 10 colors`).toHaveLength(10);
expect(
palette.colors,
`palette "${id}" should have 10 colors`,
).toHaveLength(10);
}
});

Expand All @@ -40,31 +43,30 @@ describe("COLOR_PALETTES", () => {
expect(COLOR_PALETTES["deep-ocean"].label).toContain("Default");
});

it("contains 'warm-sunset' palette", () => {
expect(COLOR_PALETTES["warm-sunset"]).toBeDefined();
it("contains 'tableau' palette", () => {
expect(COLOR_PALETTES["tableau"]).toBeDefined();
});

it("contains 'cool-breeze' palette", () => {
expect(COLOR_PALETTES["cool-breeze"]).toBeDefined();
it("contains 'observable' palette", () => {
expect(COLOR_PALETTES["observable"]).toBeDefined();
});

it("contains 'earth-tones' palette", () => {
expect(COLOR_PALETTES["earth-tones"]).toBeDefined();
it("contains 'sequential' palette", () => {
expect(COLOR_PALETTES["sequential"]).toBeDefined();
});

it("contains 'neon' palette", () => {
expect(COLOR_PALETTES["neon"]).toBeDefined();
it("contains 'diverging' palette", () => {
expect(COLOR_PALETTES["diverging"]).toBeDefined();
});

it("contains 'monochrome' palette", () => {
expect(COLOR_PALETTES["monochrome"]).toBeDefined();
});

it("deep-ocean colors match DEEP_OCEAN_LIGHT from theme", () => {
// The deep-ocean palette should reuse the existing DEEP_OCEAN_LIGHT values
const deepOcean = COLOR_PALETTES["deep-ocean"];
expect(deepOcean.colors[0]).toBe("hsl(217, 91%, 60%)"); // Blue
expect(deepOcean.colors[1]).toBe("hsl(38, 92%, 50%)"); // Amber
expect(deepOcean.colors[1]).toBe("hsl(38, 92%, 50%)"); // Amber
});
});

Expand All @@ -79,7 +81,10 @@ describe("getPaletteColors", () => {
it("returns colors for all defined palettes", () => {
for (const id of Object.keys(COLOR_PALETTES)) {
const colors = getPaletteColors(id);
expect(colors, `getPaletteColors("${id}") should return colors`).toBeDefined();
expect(
colors,
`getPaletteColors("${id}") should return colors`,
).toBeDefined();
expect(colors!.length).toBe(10);
}
});
Expand All @@ -89,14 +94,13 @@ describe("getPaletteColors", () => {
});

it("returns the same reference as COLOR_PALETTES[id].colors", () => {
const colors = getPaletteColors("warm-sunset");
expect(colors).toBe(COLOR_PALETTES["warm-sunset"].colors);
const colors = getPaletteColors("tableau");
expect(colors).toBe(COLOR_PALETTES["tableau"].colors);
});
});

describe("ColorPalette type structure", () => {
it("satisfies the ColorPalette interface shape", () => {
// This is a compile-time check validated at runtime
const palette: ColorPalette = { label: "Test", colors: ["#fff"] };
expect(palette.label).toBe("Test");
expect(palette.colors).toHaveLength(1);
Expand Down
Loading
Loading