test(app): add widget-editor sub-component tests (#578) - #628
Merged
Conversation
WalkthroughAdds four new Vitest + React Testing Library suites for widget-editor: chart-type-selector, form-fields-editor, parameter-config-section-ui, and parameter-config-section (mapping helpers). All changes are tests only; no runtime exports or public APIs were modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/widget-editor/__tests__/parameter-config-section.test.tsx (1)
1-129: Rename this suite to.test.ts.This file only exercises pure helpers, so
.test.tsxis misleading and violates the app test convention for non-UI utils. Please move it to.test.tsunless you plan to add JSX/rendering here. As per coding guidelines, unit tests for pure functions and utils in app/ use Vitest (no DOM) with.test.tsfile extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/__tests__/parameter-config-section.test.tsx` around lines 1 - 129, The test file is named .test.tsx but only exercises pure helpers (resolveInternalParamType, reverseParamTypeMapping) and should be a non-DOM Vitest test; rename the file from parameter-config-section.test.tsx to parameter-config-section.test.ts (or create a new .test.ts and move the contents) so it follows the app convention for pure-function tests, and ensure imports/vi.mock usage remain valid after the rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/components/widget-editor/__tests__/parameter-config-section.test.tsx`:
- Around line 1-129: The test file is named .test.tsx but only exercises pure
helpers (resolveInternalParamType, reverseParamTypeMapping) and should be a
non-DOM Vitest test; rename the file from parameter-config-section.test.tsx to
parameter-config-section.test.ts (or create a new .test.ts and move the
contents) so it follows the app convention for pure-function tests, and ensure
imports/vi.mock usage remain valid after the rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: faa18cf2-ccda-4424-8bc1-e3b809c798b9
📒 Files selected for processing (4)
app/src/components/widget-editor/__tests__/chart-type-selector.test.tsxapp/src/components/widget-editor/__tests__/form-fields-editor.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section-ui.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section.test.tsx
2 tasks
Add 55 unit tests across 4 test files covering: - resolveInternalParamType / reverseParamTypeMapping pure functions (16 tests) - ChartTypeSelector component rendering and callbacks (10 tests) - ParameterConfigSection UI: all param types, seed query, collisions (18 tests) - FormFieldsEditor: empty state, add/remove fields, DnD mocks (11 tests) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
force-pushed
the
test/issue-578-widget-editor-coverage
branch
from
May 1, 2026 15:18
30d4c69 to
60e6a39
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/widget-editor/__tests__/chart-type-selector.test.tsx (1)
44-52: ⚡ Quick winStrengthen icon assertions by testing concrete fallback identity
At Line 44, every
lucide-reactexport is mocked to the same component, so the assertions at Lines 65 and 70 only prove “defined,” not that known vs fallback icon resolution is correct. This can miss regressions inchartTypeIcons[type] ?? Braces.Suggested test tightening
import React from "react"; import { describe, it, expect, vi } from "vitest"; import { render, screen, fireEvent } from "@testing-library/react"; +import { Braces } from "lucide-react"; @@ -vi.mock("lucide-react", async (importOriginal) => { - const Icon = () => <span />; - const actual = await importOriginal<Record<string, unknown>>(); - const mocked: Record<string, unknown> = {}; - for (const key of Object.keys(actual)) { - mocked[key] = Icon; - } - return mocked; -}); @@ describe("getChartTypeMeta", () => { it("returns label from chart config and icon from map", () => { const meta = getChartTypeMeta("bar"); expect(meta.label).toBe("Bar"); - expect(meta.Icon).toBeDefined(); + expect(meta.Icon).toBe(chartTypeIcons.bar); }); it("returns fallback icon for unknown type", () => { const meta = getChartTypeMeta("unknown-type" as ChartType); - expect(meta.Icon).toBeDefined(); + expect(meta.Icon).toBe(Braces); }); });Also applies to: 62-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/widget-editor/__tests__/chart-type-selector.test.tsx` around lines 44 - 52, The test currently mocks every lucide-react export to the same component which prevents asserting that chartTypeIcons[type] resolves to a specific known icon vs the fallback Braces; update the vi.mock in chart-type-selector.test.tsx so that known icon exports (the specific names used in chartTypeIcons) return distinct, identifiable mock components and all other icons return a generic fallback mock (e.g., createNamedMock('KnownIcon') vs createNamedMock('Fallback')), then change the assertions that currently check for defined to assert identity (strict equality or matching displayName) between the rendered icon and the expected symbol (chartTypeIcons[type] or Braces) to ensure correct resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/components/widget-editor/__tests__/chart-type-selector.test.tsx`:
- Around line 44-52: The test currently mocks every lucide-react export to the
same component which prevents asserting that chartTypeIcons[type] resolves to a
specific known icon vs the fallback Braces; update the vi.mock in
chart-type-selector.test.tsx so that known icon exports (the specific names used
in chartTypeIcons) return distinct, identifiable mock components and all other
icons return a generic fallback mock (e.g., createNamedMock('KnownIcon') vs
createNamedMock('Fallback')), then change the assertions that currently check
for defined to assert identity (strict equality or matching displayName) between
the rendered icon and the expected symbol (chartTypeIcons[type] or Braces) to
ensure correct resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9e43746-9176-4bd8-b5bf-d197f2e5c9e3
📒 Files selected for processing (4)
app/src/components/widget-editor/__tests__/chart-type-selector.test.tsxapp/src/components/widget-editor/__tests__/form-fields-editor.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section-ui.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/components/widget-editor/tests/parameter-config-section.test.tsx
- app/src/components/widget-editor/tests/parameter-config-section-ui.test.tsx
- app/src/components/widget-editor/tests/form-fields-editor.test.tsx
|
This was referenced May 2, 2026
This was referenced May 12, 2026
6 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
parameter-config-section.test.tsx— 16 tests forresolveInternalParamTypeandreverseParamTypeMappingpure functions including roundtrip verificationchart-type-selector.test.tsx— 10 tests for rendering, connection/chart-type selection callbacks, and icon map completenessparameter-config-section-ui.test.tsx— 18 tests for all param types (date/freetext/select), seed query states, collision banners, reference hintsform-fields-editor.test.tsx— 11 tests for empty state, add/remove fields, field metadata display, DnD kit mockedTest plan
tlsmodule resolution issue (not related to this PR)Closes #578
🤖 Generated with Claude Code
Summary by CodeRabbit