Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/frontend/src/components/tool-calls/display-kpi.tsx">
<violation number="1" location="apps/frontend/src/components/tool-calls/display-kpi.tsx:36">
P0: **Rules of Hooks violation**: `useMemo` is called after two conditional early returns. Hooks must be called unconditionally and in the same order on every render. This will cause a React runtime crash when the component transitions between loading/error and resolved states.
Move the `useMemo` call above all early returns (and guard against `config` being undefined inside the memo).</violation>
</file>
<file name="apps/frontend/src/components/side-panel/story-kpi-embed.tsx">
<violation number="1" location="apps/frontend/src/components/side-panel/story-kpi-embed.tsx:25">
P2: Bug: `break` only exits the inner loop over `message.parts`, not the outer loop over `messages`. If the same `queryId` appears in a later message, the previously resolved value will be overwritten. The sibling component `story-chart-embed.tsx` avoids this by using `return` to exit both loops. Consider using a labeled break or refactoring to match the `story-chart-embed.tsx` pattern.</violation>
</file>
<file name="apps/frontend/src/routes/_sidebar-layout.stories.shared.$shareId.tsx">
<violation number="1" location="apps/frontend/src/routes/_sidebar-layout.stories.shared.$shareId.tsx:148">
P1: Runtime crash: `result` can be `undefined` (optional chaining + `as` cast doesn't provide runtime safety), and `result[0]` can be `undefined` for empty arrays. Accessing `.column` on either case throws a `TypeError`. Add null/empty checks similar to `SharedChartEmbed`.</violation>
</file>
<file name="apps/frontend/src/components/side-panel/story-editor.tsx">
<violation number="1" location="apps/frontend/src/components/side-panel/story-editor.tsx:156">
P2: Copy-paste bug: error fallback uses `data-type='chart-block'` instead of `data-type='kpi-block'`. The success branch correctly uses `'kpi-block'`.</violation>
</file>
<file name="apps/frontend/src/lib/stories-page.ts">
<violation number="1" location="apps/frontend/src/lib/stories-page.ts:220">
P2: `segment.type` is a string literal that always equals `'kpi'` — this is not meaningful text for search filtering. It means every story containing a KPI segment will match a search for "kpi", but no actual KPI content is searchable. Consider returning an empty string (since the `kpi` segment type has no text/title field), or a descriptive string if desired.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
| } | ||
|
|
||
| const resolvedKpis = useMemo( |
There was a problem hiding this comment.
P0: Rules of Hooks violation: useMemo is called after two conditional early returns. Hooks must be called unconditionally and in the same order on every render. This will cause a React runtime crash when the component transitions between loading/error and resolved states.
Move the useMemo call above all early returns (and guard against config being undefined inside the memo).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/frontend/src/components/tool-calls/display-kpi.tsx, line 36:
<comment>**Rules of Hooks violation**: `useMemo` is called after two conditional early returns. Hooks must be called unconditionally and in the same order on every render. This will cause a React runtime crash when the component transitions between loading/error and resolved states.
Move the `useMemo` call above all early returns (and guard against `config` being undefined inside the memo).</comment>
<file context>
@@ -0,0 +1,93 @@
+ );
+ }
+
+ const resolvedKpis = useMemo(
+ () =>
+ config.kpis.map((kpi) => {
</file context>
| const resolvedKpis = kpis.map((kpi) => { | ||
| console.log(queryData); | ||
| const result = queryData?.[kpi.queryId]?.data as Record<string, unknown>[]; | ||
| const value = result[0][kpi.column] ?? null; |
There was a problem hiding this comment.
P1: Runtime crash: result can be undefined (optional chaining + as cast doesn't provide runtime safety), and result[0] can be undefined for empty arrays. Accessing .column on either case throws a TypeError. Add null/empty checks similar to SharedChartEmbed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/frontend/src/routes/_sidebar-layout.stories.shared.$shareId.tsx, line 148:
<comment>Runtime crash: `result` can be `undefined` (optional chaining + `as` cast doesn't provide runtime safety), and `result[0]` can be `undefined` for empty arrays. Accessing `.column` on either case throws a `TypeError`. Add null/empty checks similar to `SharedChartEmbed`.</comment>
<file context>
@@ -124,6 +134,30 @@ function SharedChartEmbed({
+ const resolvedKpis = kpis.map((kpi) => {
+ console.log(queryData);
+ const result = queryData?.[kpi.queryId]?.data as Record<string, unknown>[];
+ const value = result[0][kpi.column] ?? null;
+ return { kpi, value };
+ });
</file context>
| const value = result[0][kpi.column] ?? null; | |
| const value = result?.[0]?.[kpi.column] ?? null; |
| if (outputData.length === 1 && part.output.columns.includes(kpi.column)) { | ||
| value = outputData[0][kpi.column] ?? null; | ||
| } | ||
| break; |
There was a problem hiding this comment.
P2: Bug: break only exits the inner loop over message.parts, not the outer loop over messages. If the same queryId appears in a later message, the previously resolved value will be overwritten. The sibling component story-chart-embed.tsx avoids this by using return to exit both loops. Consider using a labeled break or refactoring to match the story-chart-embed.tsx pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/frontend/src/components/side-panel/story-kpi-embed.tsx, line 25:
<comment>Bug: `break` only exits the inner loop over `message.parts`, not the outer loop over `messages`. If the same `queryId` appears in a later message, the previously resolved value will be overwritten. The sibling component `story-chart-embed.tsx` avoids this by using `return` to exit both loops. Consider using a labeled break or refactoring to match the `story-chart-embed.tsx` pattern.</comment>
<file context>
@@ -0,0 +1,41 @@
+ if (outputData.length === 1 && part.output.columns.includes(kpi.column)) {
+ value = outputData[0][kpi.column] ?? null;
+ }
+ break;
+ }
+ }
</file context>
|
|
||
| if (kpis.length === 0) { | ||
| return ( | ||
| <NodeViewWrapper draggable data-type='chart-block'> |
There was a problem hiding this comment.
P2: Copy-paste bug: error fallback uses data-type='chart-block' instead of data-type='kpi-block'. The success branch correctly uses 'kpi-block'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/frontend/src/components/side-panel/story-editor.tsx, line 156:
<comment>Copy-paste bug: error fallback uses `data-type='chart-block'` instead of `data-type='kpi-block'`. The success branch correctly uses `'kpi-block'`.</comment>
<file context>
@@ -133,6 +139,84 @@ const ChartBlock = Node.create({
+
+ if (kpis.length === 0) {
+ return (
+ <NodeViewWrapper draggable data-type='chart-block'>
+ <div className='my-2 rounded-lg border border-dashed p-4 text-center text-sm text-muted-foreground'>
+ Invalid KPI block
</file context>
| <NodeViewWrapper draggable data-type='chart-block'> | |
| <NodeViewWrapper draggable data-type='kpi-block'> |
| case 'table': | ||
| return segment.title; | ||
| case 'kpi': | ||
| return segment.type; |
There was a problem hiding this comment.
P2: segment.type is a string literal that always equals 'kpi' — this is not meaningful text for search filtering. It means every story containing a KPI segment will match a search for "kpi", but no actual KPI content is searchable. Consider returning an empty string (since the kpi segment type has no text/title field), or a descriptive string if desired.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/frontend/src/lib/stories-page.ts, line 220:
<comment>`segment.type` is a string literal that always equals `'kpi'` — this is not meaningful text for search filtering. It means every story containing a KPI segment will match a search for "kpi", but no actual KPI content is searchable. Consider returning an empty string (since the `kpi` segment type has no text/title field), or a descriptive string if desired.</comment>
<file context>
@@ -216,6 +216,8 @@ function extractSegmentText(segment: SummarySegment): string {
case 'table':
return segment.title;
+ case 'kpi':
+ return segment.type;
case 'grid':
return segment.children.map(extractSegmentText).join(' ');
</file context>
| return segment.type; | |
| return ''; |
|
Hello! Thank you for your first contribution! KPI card should not be a tool but more a chart subtype of the current charts. If we create tools for different kind of visualisation it gonna add so much noise to the context windows. Second point is: I think the style of the card should be improved, card looks like way too much like the default vibe coded dashboards cards we have on every Lovable apps. |
2 participants
Summary
Examples
KPI Cards in Chat
KPI Cards in Story Side Panel
KPI Cards in Story Side Panel Editor
KPI Cards in Story Thumbnail
KPI Cards In Story Viewer