improve: widgets — orphaned widget handling, duplicate template, test query - #296
Merged
Conversation
- Add "Duplicate" button (Copy icon) on template cards — creates a copy with "(copy)" suffix via existing createTemplate mutation - Add "Test Query" button (Play icon) on templates that have a query and connection — executes the query via /api/query and shows success/error result - Test query button disabled while running, shows loading state Closes #270 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…orphaned-widgets # Conflicts: # app/src/lib/__tests__/chart-registry.test.ts
…-duplicate-template
…-duplicate-template
…-duplicate-template
…e' into improve/widgets-integration
|
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)
WalkthroughAdds Duplicate and Test Query actions to Widget Lab template cards, extracts chart click interaction logic into a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TemplateCard (UI)
participant Page as WidgetLabPage
participant API as /api/query
participant Toast as useToast
UI->>Page: onTestQuery(template)
Page->>API: POST /api/query {connectionId, query, params}
API-->>Page: 200 / error
Page->>Toast: show success / error
Page-->>UI: clear loading state
sequenceDiagram
participant UI as TemplateCard (UI)
participant Page as WidgetLabPage
participant API as createTemplate.mutate (client hook)
participant Store as Backend DB
UI->>Page: onDuplicate(template)
Page->>API: createTemplate.mutate({name: copiedName, description, tags, ...})
API->>Store: persist template
Store-->>API: created template
API-->>Page: mutation success
Page->>UI: update list (refetch/optimistic)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Page components (app/**/page.tsx) are UI-heavy React Server/Client components that require full browser mounts to test. Their logic is covered via E2E tests, not unit tests. Excluding them prevents false coverage gaps on PRs that modify page-level handlers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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-706:⚠️ Potential issue | 🟠 MajorComponent loaders are not SSR-safe and appear unused.
Per the relevant code snippet,
chart-renderer.tsxlines 18-105 and 188-240 show a hardcoded switch statement with its owndynamic()imports — it does not callgetChartConfig(type).component(). These loaders are dead code until the renderer is refactored.Additionally, when this is wired up, each loader needs SSR disabled. Consider either:
- Return pre-wrapped dynamic components (changes signature):
import dynamic from "next/dynamic"; component: dynamic( () => import("@neoboard/components").then((m) => ({ default: m.BarChart })), { ssr: false } ),
- Document that the consumer must wrap with
dynamic({ ssr: false })and update chart-renderer to do so.As per coding guidelines: "Chart components must use next/dynamic with ssr: false".
🤖 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 - 706, The component loader entries in chart-registry.ts (the component field returned by getChartConfig/type entries such as bar -> component and line -> component) are currently plain import factories, unused by chart-renderer.tsx's hardcoded switch and unsafe for SSR; update either the registry or the renderer so chart components are provided as Next dynamic components with ssr: false: either change the registry's component value to return pre-wrapped dynamic components (thus changing the signature of component entries) or modify chart-renderer.tsx to call getChartConfig(type).component and wrap the returned loader with next/dynamic({ ssr: false }) before rendering; ensure you also remove or wire-up any dead loaders in chart-registry.ts so all chart types (e.g., BarChart, LineChart) follow the "Chart components must use next/dynamic with ssr: false" guideline.
🧹 Nitpick comments (1)
app/src/app/(dashboard)/widget-lab/page.tsx (1)
274-278: Consider replacingalert()with a toast notification.Native
alert()blocks the main thread and cannot be styled. A toast would provide better UX and align with the PR objective of "appropriate UI states."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/app/`(dashboard)/widget-lab/page.tsx around lines 274 - 278, Replace the blocking alert() calls in the query response handling with non-blocking toast notifications: instead of alert("Query executed successfully."), call your app's toast success method (e.g., toast.success("Query executed successfully.")), and instead of alert(`Query failed: ${err.error?.message ?? res.statusText}`) call toast.error(...) with the same message; import and use the project's toast utility (or add one like react-hot-toast) at the top of the file and ensure you surface err.error?.message ?? res.statusText in the toast error message so the user sees the same error details.
🤖 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/app/`(dashboard)/widget-lab/page.tsx:
- Around line 273-282: The current error handling calls await res.json()
directly and will throw if the response body isn't JSON; update the non-OK
branch that uses the res variable to attempt JSON parsing in a try/catch (e.g.,
try { const err = await res.json(); ... } catch { const text = await
res.text().catch(() => null); ... }) and then use the parsed err.error?.message
if available, else fall back to the plain text or res.statusText so the alert
shows a meaningful message for non-JSON responses; make this change around the
existing if (res.ok) { ... } else { ... } block.
In `@app/src/hooks/__tests__/use-click-action.test.ts`:
- Around line 63-66: The test currently uses a conditional guard "if
(result?.setParameter)" which can let the test pass with zero assertions;
instead assert that result.setParameter exists explicitly (e.g.,
expect(result?.setParameter).toBeDefined()/not.toBeNull()) before checking its
fields, then assert result.setParameter.parameterName and
result.setParameter.value; update the test referencing the same symbols (result
and setParameter) so missing setParameter fails the test rather than silently
skipping assertions.
- Around line 23-27: Tests only assert the exported type of useClickAction and
never exercise the hook or its internal handler, so add tests that mount the
hook (e.g., using renderHook or a test component) to obtain the returned values,
call the returned handleChartClick with representative event/payloads, and
assert the expected side effects (callback invocation, store updates, or
dispatched actions) and cleanup; specifically locate and exercise useClickAction
and its returned handleChartClick, mock any dependencies/callbacks (e.g.,
onClick handlers or store selectors) and verify they are invoked with correct
args and that the hook wiring works end-to-end.
In `@app/src/lib/chart-registry.ts`:
- Around line 44-52: The `component` field in chart-registry.ts is dead or
incorrectly implemented: either remove the `component: () => Promise<{ default:
React.ComponentType<any> }>` property from the registry entries, or refactor it
to return a next/dynamic loader that disables SSR and then update chart-renderer
to consume that loader instead of its hardcoded dynamic imports; specifically,
if keeping it, make `component` provide a next/dynamic(...) result with { ssr:
false } (so chart-renderer can render without SSR) and ensure chart-renderer
uses the registry's `component` property rather than its internal imports,
otherwise delete all references to `component` and any comments claiming
chart-renderer uses it.
---
Outside diff comments:
In `@app/src/lib/chart-registry.ts`:
- Around line 692-706: The component loader entries in chart-registry.ts (the
component field returned by getChartConfig/type entries such as bar -> component
and line -> component) are currently plain import factories, unused by
chart-renderer.tsx's hardcoded switch and unsafe for SSR; update either the
registry or the renderer so chart components are provided as Next dynamic
components with ssr: false: either change the registry's component value to
return pre-wrapped dynamic components (thus changing the signature of component
entries) or modify chart-renderer.tsx to call getChartConfig(type).component and
wrap the returned loader with next/dynamic({ ssr: false }) before rendering;
ensure you also remove or wire-up any dead loaders in chart-registry.ts so all
chart types (e.g., BarChart, LineChart) follow the "Chart components must use
next/dynamic with ssr: false" guideline.
---
Nitpick comments:
In `@app/src/app/`(dashboard)/widget-lab/page.tsx:
- Around line 274-278: Replace the blocking alert() calls in the query response
handling with non-blocking toast notifications: instead of alert("Query executed
successfully."), call your app's toast success method (e.g.,
toast.success("Query executed successfully.")), and instead of alert(`Query
failed: ${err.error?.message ?? res.statusText}`) call toast.error(...) with the
same message; import and use the project's toast utility (or add one like
react-hot-toast) at the top of the file and ensure you surface
err.error?.message ?? res.statusText in the toast error message so the user sees
the same error details.
🪄 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: 544ab259-6ec3-4e47-8e82-ccaa1715eea4
📒 Files selected for processing (6)
app/src/app/(dashboard)/widget-lab/page.tsxapp/src/components/card-container.tsxapp/src/hooks/__tests__/use-click-action.test.tsapp/src/hooks/use-click-action.tsapp/src/lib/chart-registry.tssonar-project.properties
Comment on lines
+273
to
+282
| if (res.ok) { | ||
| alert("Query executed successfully."); | ||
| } else { | ||
| const err = await res.json(); | ||
| alert(`Query failed: ${err.error?.message ?? res.statusText}`); | ||
| } | ||
| } catch (e) { | ||
| alert( | ||
| `Query failed: ${e instanceof Error ? e.message : "Unknown error"}`, | ||
| ); |
There was a problem hiding this comment.
Guard against non-JSON error responses.
If the server returns a non-JSON response (e.g., a 502 gateway error with HTML), res.json() will throw, and the user will see a cryptic "Unexpected token" message instead of the actual status.
Proposed fix
if (res.ok) {
alert("Query executed successfully.");
} else {
- const err = await res.json();
- alert(`Query failed: ${err.error?.message ?? res.statusText}`);
+ let message = res.statusText;
+ try {
+ const err = await res.json();
+ message = err.error?.message ?? message;
+ } catch {
+ // Response wasn't JSON — use statusText
+ }
+ alert(`Query failed: ${message}`);
}📝 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 (res.ok) { | |
| alert("Query executed successfully."); | |
| } else { | |
| const err = await res.json(); | |
| alert(`Query failed: ${err.error?.message ?? res.statusText}`); | |
| } | |
| } catch (e) { | |
| alert( | |
| `Query failed: ${e instanceof Error ? e.message : "Unknown error"}`, | |
| ); | |
| if (res.ok) { | |
| alert("Query executed successfully."); | |
| } else { | |
| let message = res.statusText; | |
| try { | |
| const err = await res.json(); | |
| message = err.error?.message ?? message; | |
| } catch { | |
| // Response wasn't JSON — use statusText | |
| } | |
| alert(`Query failed: ${message}`); | |
| } | |
| } catch (e) { | |
| alert( | |
| `Query failed: ${e instanceof Error ? e.message : "Unknown error"}`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/app/`(dashboard)/widget-lab/page.tsx around lines 273 - 282, The
current error handling calls await res.json() directly and will throw if the
response body isn't JSON; update the non-OK branch that uses the res variable to
attempt JSON parsing in a try/catch (e.g., try { const err = await res.json();
... } catch { const text = await res.text().catch(() => null); ... }) and then
use the parsed err.error?.message if available, else fall back to the plain text
or res.statusText so the alert shows a meaningful message for non-JSON
responses; make this change around the existing if (res.ok) { ... } else { ... }
block.
Comment on lines
+44
to
+52
| /** | ||
| * Lazy component loader for this chart type. Used by chart-renderer | ||
| * to dynamically import the component. Returns a module with a default export. | ||
| * | ||
| * For charts that don't need lazy loading (e.g., JSON, Markdown), this | ||
| * can return the component directly wrapped in `{ default: Component }`. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| component: () => Promise<{ default: React.ComponentType<any> }>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the registry's component field is actually used
rg -nP --type=ts --type=tsx '\.component\s*\(' -g '!chart-registry.ts'
rg -nP --type=ts 'getChartConfig.*\.component'
ast-grep --pattern 'getChartConfig($_).component'Repository: alfredo1996/neoboard
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search with correct syntax - rg supports -t ts for TypeScript files
rg '\.component\s*\(' --type ts -g '!chart-registry.ts'
# Also search for direct component field access
rg 'getChartConfig.*\.component' --type ts
# Search for imports or references to component field
rg '\bcomponent\b' app/src/lib/ --type ts -B 2 -A 2 | head -100Repository: alfredo1996/neoboard
Length of output: 5717
🏁 Script executed:
#!/bin/bash
# Check how chart-renderer.tsx handles components
rg -l 'chart-renderer' --type ts app/src/
cat app/src/lib/chart-renderer.tsx 2>/dev/null | head -150Repository: alfredo1996/neoboard
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Read chart-renderer.tsx to see how it consumes the registry
cat app/src/components/chart-renderer.tsx | head -200Repository: alfredo1996/neoboard
Length of output: 6625
🏁 Script executed:
#!/bin/bash
# Search for any usage of getChartConfig or registry component in chart-renderer
rg 'getChartConfig|component.*from.*registry|\.component' app/src/components/chart-renderer.tsx --type ts -B 3 -A 3Repository: alfredo1996/neoboard
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search entire codebase for any reference to .component on chart config objects
rg 'config\.component|getChartConfig.*\.component|ChartConfig.*\.component' --type ts
# Also check if there are any imports or references to this field specifically
rg '\bcomponent\b.*Promise.*default' app/src --type ts | grep -v chart-registryRepository: alfredo1996/neoboard
Length of output: 46
Remove unused component field or properly integrate it with next/dynamic.
The component loaders in chart-registry.ts use plain import() without next/dynamic({ ssr: false }), violating the requirement that chart components must disable SSR. Additionally, the comment claiming this field is "used by chart-renderer" is inaccurate—chart-renderer uses its own hardcoded dynamic imports instead. The field is dead code and should either be removed or refactored to be properly consumed by chart-renderer with SSR disabled.
🤖 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 44 - 52, The `component` field in
chart-registry.ts is dead or incorrectly implemented: either remove the
`component: () => Promise<{ default: React.ComponentType<any> }>` property from
the registry entries, or refactor it to return a next/dynamic loader that
disables SSR and then update chart-renderer to consume that loader instead of
its hardcoded dynamic imports; specifically, if keeping it, make `component`
provide a next/dynamic(...) result with { ssr: false } (so chart-renderer can
render without SSR) and ensure chart-renderer uses the registry's `component`
property rather than its internal imports, otherwise delete all references to
`component` and any comments claiming chart-renderer uses it.
…p copy suffix - Add AbortSignal.timeout(30_000) to handleTestQuery fetch call - Replace all alert() calls with toast notifications (useToast from @neoboard/components) - Handle TimeoutError specifically with a dedicated message - Add handleDuplicate with (copy) suffix deduplication via regex strip - Add onSuccess/onError callbacks to createTemplate.mutate in handleDuplicate Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
…s-integration # Conflicts: # app/src/lib/__tests__/chart-registry.test.ts
This was referenced Apr 12, 2026
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
improve: widgets — orphaned widget handling, duplicate template, test query
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 combining widget PRs:
Closes #270, #271
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores