improve: app flow — registry component loaders + useClickAction hook - #281
Merged
Conversation
Closes #255 - Extract click action handling (resolveClickActions, setParameter, deriveClickableColumns) into reusable useClickAction hook - card-container.tsx reduced by 25 lines — uses hook instead of inline logic - Remove unused imports (ClickAction type, resolveClickActions, etc.) - 7 unit tests for the hook logic (TDD: RED → GREEN) Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes #254 Each ChartConfig now has a `component` field — a lazy loader function that returns the chart's React component. This is the first step toward eliminating the switch statement in chart-renderer.tsx. Currently: registry owns component references + chart-renderer switch. Next step: chart-renderer uses registry.component via next/dynamic to render, removing the switch entirely. - Add `component` field to ChartConfig interface - Add lazy loaders for all 17 chart types - Add test verifying every chart type has a component field Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughCentralized widget click-action handling into a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Card as CardContainer
participant Hook as useClickAction
participant Store as ParameterStore
participant Nav as Navigation
participant Chart as ChartRenderer
User->>Card: click chart point
Card->>Hook: handleChartClick(point)
Hook->>Hook: resolveClickActions(widget, point)
alt result.setParameter exists
Hook->>Store: setParameter(name, value, metadata)
end
alt result.navigateToPageId exists
Hook->>Nav: onNavigateToPage(pageId, scrollToWidgetId?)
end
Hook->>Card: (returns)
Card->>Chart: (click handled) update UI as needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/lib/chart-registry.ts (1)
692-787:⚠️ Potential issue | 🟠 MajorUse the charts sub-barrel instead of the main package barrel to avoid pulling unnecessary heavy dependencies.
The main
@neoboard/componentsbarrel re-exports all charts including MapChart and GraphChart. Sincecomponent/src/charts/map-chart.tsximports Leaflet andcomponent/src/charts/graph-chart.tsximports NVL, lazy-loading any chart through the main barrel will pull both heavy dependencies into every async chunk, negating the lazy-load benefit.Change loaders to import from
@neoboard/components/charts(or specify the leaf paths) so that BarChart, LineChart, etc. don't bundle map and graph code:component: () => import("@neoboard/components/charts").then((m) => ({ default: m.BarChart })),This ensures heavy deps load only when their respective widgets are present on the dashboard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/chart-registry.ts` around lines 692 - 787, The component loaders in chart registry (e.g., the component arrow functions that import "@neoboard/components" for BarChart, LineChart, PieChart, TableRenderer, SingleValueChart, MapChart, GraphChart) are pulling the full package barrel and thus bundling heavy Map/Graph deps; update those lazy imports to use the charts sub-barrel (import "@neoboard/components/charts") or direct chart leaf paths for lightweight charts (BarChart, LineChart, PieChart, TableRenderer, SingleValueChart) while keeping MapChart and GraphChart pointing to their full/leaf modules so Leaflet/NVL only load when those charts are used; replace the import specifier in each component: () => import("@neoboard/components")... with () => import("@neoboard/components/charts")... (or equivalent leaf paths) for non-map/graph chart entries.
🤖 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/hooks/__tests__/use-click-action.test.ts`:
- Around line 7-89: The tests currently only assert mocked outputs of
resolveClickActions and deriveClickableColumns and never import or exercise the
hook logic; update the test to actually import useClickAction and call the
returned handler (e.g., the handleChartClick function or whatever handler the
hook returns), wiring a real or mocked parameter store (setParameter) and
ensuring resolveClickActions/deriveClickableColumns are mocked only to drive
inputs — or alternatively convert this file into a pure unit test for the helper
functions (resolveClickActions/deriveClickableColumns) and remove hook mentions;
specifically, import useClickAction, obtain the click handler from
useClickAction, call it with a sample payload, and assert that the handler
invokes setParameter with the expected args or navigates via the returned
navigateToPageId, while still mocking resolveClickActions/deriveClickableColumns
to control behavior.
In `@app/src/lib/__tests__/chart-registry.test.ts`:
- Around line 1833-1836: The test dereferences config.component but
getChartConfig(type) can return undefined; narrow config first by asserting it's
defined or throwing if undefined (e.g., use expect(config).toBeDefined() or if
(!config) fail()) before checking config.component, then assert typeof
config.component === "function"; update the test in chart-registry.test.ts (the
it.each block using getChartConfig and config.component) to include that
null-check/expect to avoid the strict-null error.
---
Outside diff comments:
In `@app/src/lib/chart-registry.ts`:
- Around line 692-787: The component loaders in chart registry (e.g., the
component arrow functions that import "@neoboard/components" for BarChart,
LineChart, PieChart, TableRenderer, SingleValueChart, MapChart, GraphChart) are
pulling the full package barrel and thus bundling heavy Map/Graph deps; update
those lazy imports to use the charts sub-barrel (import
"@neoboard/components/charts") or direct chart leaf paths for lightweight charts
(BarChart, LineChart, PieChart, TableRenderer, SingleValueChart) while keeping
MapChart and GraphChart pointing to their full/leaf modules so Leaflet/NVL only
load when those charts are used; replace the import specifier in each component:
() => import("@neoboard/components")... with () =>
import("@neoboard/components/charts")... (or equivalent leaf paths) for
non-map/graph chart entries.
🪄 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: 4abfb3fc-8de2-4599-88ca-75d890c76506
📒 Files selected for processing (5)
app/src/components/card-container.tsxapp/src/hooks/__tests__/use-click-action.test.tsapp/src/hooks/use-click-action.tsapp/src/lib/__tests__/chart-registry.test.tsapp/src/lib/chart-registry.ts
- Fix TableRenderer import to use local path (component/ move is in #282) - Add null guard to getChartConfig() in component field test - Rewrite useClickAction tests to import actual hook and exercise real functions instead of only mocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add async tests that call each chart registry component() loader and verify the resolved module has a default export (mocked via vi.mock to avoid pulling real UI dependencies in unit tests) - Add renderHook tests for useClickAction that exercise the actual hook body: useCallback wiring, parameter store integration, and page navigation callback - Covers new lines flagged as 0% by SonarCloud on PR #281 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ow-integration # Conflicts: # app/src/lib/__tests__/chart-registry.test.ts
Graph and map charts now have stylingTargets (added in #282). Update tests to expect true for chartSupportsStyling and non-empty styling targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tyling docs - Add inline rationale to eslint-disable for any in component type - Add comment noting component fields will replace chart-renderer.tsx loaders - Update supportsStyling docstring to reflect graph/map now having styling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 (2)
app/src/hooks/__tests__/use-click-action.test.ts (1)
36-53: These derivation tests are redundant with hook-level coverage.The block at Line 36-53 only tests boolean casting on local objects, not hook behavior. You already validate
hasClickActionthroughrenderHookat Line 151-166. Consider removing this block or converting it to real hook assertions to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/hooks/__tests__/use-click-action.test.ts` around lines 36 - 53, Remove or replace the redundant standalone boolean-casting tests in the "useClickAction — hasClickAction derivation" describe block: either delete that entire describe block (the two it cases that create local ws objects and assert !!clickAction) or convert them into real hook tests that call the useClickAction hook via renderHook and assert the hasClickAction output; reference the hook name useClickAction, the derived value hasClickAction, and the ClickAction shape/variable used in the current tests when implementing the replacement.app/src/lib/__tests__/chart-registry.test.ts (1)
1097-1100: Add the matchingmapstyling-target assertion.
graphis pinned exactly now, butapp/src/lib/chart-registry.tsalso givesmapan explicit[{ value: "color", label: "Marker Color" }]. Without the paired assertion, regressions ingetStylingTargets("map")still slip through.Suggested addition
it("returns styling targets for graph", () => { expect(getStylingTargets("graph")).toEqual([ { value: "color", label: "Node Color" }, ]); }); + + it("returns styling targets for map", () => { + expect(getStylingTargets("map")).toEqual([ + { value: "color", label: "Marker Color" }, + ]); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/__tests__/chart-registry.test.ts` around lines 1097 - 1100, The test currently asserts getStylingTargets("graph") but misses a paired assertion for getStylingTargets("map"); update the test in chart-registry.test.ts to also call getStylingTargets("map") and expect it toEqual([{ value: "color", label: "Marker Color" }]) so the explicit "map" entry defined in app/src/lib/chart-registry.ts is covered and future regressions are caught.
🤖 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/hooks/__tests__/use-click-action.test.ts`:
- Around line 61-77: The test for resolveClickActions uses a conditional guard
(if (result?.setParameter)) which can hide failures; update the test to assert
the result shape directly by removing the if and adding explicit expectations
such as expecting result and result.setParameter to be defined (or use
toHaveProperty), then assert result.setParameter.parameterName and
result.setParameter.value equal the expected values so missing or null returns
fail the test; target the resolveClickActions call and the result/setParameter
properties in your assertions.
---
Nitpick comments:
In `@app/src/hooks/__tests__/use-click-action.test.ts`:
- Around line 36-53: Remove or replace the redundant standalone boolean-casting
tests in the "useClickAction — hasClickAction derivation" describe block: either
delete that entire describe block (the two it cases that create local ws objects
and assert !!clickAction) or convert them into real hook tests that call the
useClickAction hook via renderHook and assert the hasClickAction output;
reference the hook name useClickAction, the derived value hasClickAction, and
the ClickAction shape/variable used in the current tests when implementing the
replacement.
In `@app/src/lib/__tests__/chart-registry.test.ts`:
- Around line 1097-1100: The test currently asserts getStylingTargets("graph")
but misses a paired assertion for getStylingTargets("map"); update the test in
chart-registry.test.ts to also call getStylingTargets("map") and expect it
toEqual([{ value: "color", label: "Marker Color" }]) so the explicit "map" entry
defined in app/src/lib/chart-registry.ts is covered and future regressions are
caught.
🪄 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: b4eb8020-a5f3-40e0-854c-3231008fd9ec
📒 Files selected for processing (4)
app/src/components/card-container.tsxapp/src/hooks/__tests__/use-click-action.test.tsapp/src/lib/__tests__/chart-registry.test.tsapp/src/lib/chart-registry.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/components/card-container.tsx
- app/src/lib/chart-registry.ts
Comment on lines
+61
to
+77
| it("resolveClickActions returns setParameter with parameterName and value", () => { | ||
| const widget = { | ||
| id: "w1", | ||
| settings: { | ||
| clickAction: { | ||
| type: "set-parameter", | ||
| parameterMapping: { parameterName: "region", sourceField: "name" }, | ||
| }, | ||
| }, | ||
| } as unknown as DashboardWidget; | ||
|
|
||
| const result = resolveClickActions(widget, { name: "US", value: 100 }); | ||
| if (result?.setParameter) { | ||
| expect(result.setParameter.parameterName).toBe("region"); | ||
| expect(result.setParameter.value).toBe("US"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Avoid conditional assertions that can silently pass.
At Line 73, the if (result?.setParameter) guard can make the test pass even when resolveClickActions returns null or misses setParameter. Assert the shape directly so failures are surfaced.
Suggested fix
const result = resolveClickActions(widget, { name: "US", value: 100 });
- if (result?.setParameter) {
- expect(result.setParameter.parameterName).toBe("region");
- expect(result.setParameter.value).toBe("US");
- }
+ expect(result).toEqual(
+ expect.objectContaining({
+ setParameter: expect.objectContaining({
+ parameterName: "region",
+ value: "US",
+ }),
+ }),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/hooks/__tests__/use-click-action.test.ts` around lines 61 - 77, The
test for resolveClickActions uses a conditional guard (if
(result?.setParameter)) which can hide failures; update the test to assert the
result shape directly by removing the if and adding explicit expectations such
as expecting result and result.setParameter to be defined (or use
toHaveProperty), then assert result.setParameter.parameterName and
result.setParameter.value equal the expected values so missing or null returns
fail the test; target the resolveClickActions call and the result/setParameter
properties in your assertions.
|
This was referenced Apr 5, 2026
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
- Fix TableRenderer import to use local path (component/ move is in #282) - Add null guard to getChartConfig() in component field test - Rewrite useClickAction tests to import actual hook and exercise real functions instead of only mocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
pushed a commit
that referenced
this pull request
May 10, 2026
- Add async tests that call each chart registry component() loader and verify the resolved module has a default export (mocked via vi.mock to avoid pulling real UI dependencies in unit tests) - Add renderHook tests for useClickAction that exercise the actual hook body: useCallback wiring, parameter store integration, and page navigation callback - Covers new lines flagged as 0% by SonarCloud on PR #281 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
improve: app flow — registry component loaders + useClickAction hook
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
Integration branch for app-flow improvements. Contains:
PRs included
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests