fix: address cube review feedback

This commit is contained in:
Bhagya Amarasinghe
2026-04-30 00:39:49 +05:30
parent 38a0d7c810
commit 48086faffc
11 changed files with 59 additions and 28 deletions
@@ -11,6 +11,7 @@ import {
InvalidInputError,
InvalidPasswordResetTokenError,
OperationNotAllowedError,
QueryExecutionError,
ResourceNotFoundError,
TooManyRequestsError,
UnknownError,
@@ -74,6 +75,7 @@ describe("isExpectedError (shared helper)", () => {
"AuthenticationError",
"OperationNotAllowedError",
"ConfigurationError",
"QueryExecutionError",
"TooManyRequestsError",
"InvalidPasswordResetTokenError",
];
@@ -93,6 +95,7 @@ describe("isExpectedError (shared helper)", () => {
{ ErrorClass: ValidationError, args: ["Invalid data"] },
{ ErrorClass: OperationNotAllowedError, args: ["Not allowed"] },
{ ErrorClass: ConfigurationError, args: ["Cube is not configured"] },
{ ErrorClass: QueryExecutionError, args: ["Cube query failed. Details: connect ECONNREFUSED"] },
{ ErrorClass: InvalidPasswordResetTokenError, args: [INVALID_PASSWORD_RESET_TOKEN_ERROR_CODE] },
])("returns true for $ErrorClass.name", ({ ErrorClass, args }) => {
const error = new (ErrorClass as any)(...args);
@@ -188,6 +191,14 @@ describe("actionClient handleServerError", () => {
expect(Sentry.captureException).not.toHaveBeenCalled();
});
test("QueryExecutionError returns its message and is not sent to Sentry", async () => {
const result = await executeThrowingAction(
new QueryExecutionError("Cube query failed. Details: connect ECONNREFUSED")
);
expect(result?.serverError).toBe("Cube query failed. Details: connect ECONNREFUSED");
expect(Sentry.captureException).not.toHaveBeenCalled();
});
test("InvalidPasswordResetTokenError returns its message and is not sent to Sentry", async () => {
const result = await executeThrowingAction(
new InvalidPasswordResetTokenError(INVALID_PASSWORD_RESET_TOKEN_ERROR_CODE)
@@ -71,7 +71,7 @@ describe("executeQuery", () => {
);
});
test("wraps Cube runtime failures in a configuration error with details", async () => {
test("wraps Cube runtime failures in a query execution error with details", async () => {
mockLoad.mockRejectedValueOnce(new Error("connect ECONNREFUSED"));
const { executeQuery } = await import("./cube-client");
@@ -1,23 +1,13 @@
import cubejs, { type CubeApi, type Query } from "@cubejs-client/core";
import { ConfigurationError } from "@formbricks/types/errors";
import { ConfigurationError, QueryExecutionError } from "@formbricks/types/errors";
import { getCubeApiConfig } from "./cube-config";
const CUBE_QUERY_ERROR_MESSAGE =
"Cube query failed. Verify CUBEJS_API_URL and CUBEJS_API_SECRET, and ensure the Cube service is running.";
let cubeClient: CubeApi | null = null;
let cubeClientCacheKey: string | null = null;
function getCubeClient(): CubeApi {
const { apiSecret, apiUrl, token } = getCubeApiConfig();
const cacheKey = `${apiUrl}:${apiSecret}`;
if (!cubeClient || cubeClientCacheKey !== cacheKey) {
cubeClient = cubejs(token, { apiUrl });
cubeClientCacheKey = cacheKey;
}
return cubeClient;
const { apiUrl, token } = getCubeApiConfig();
return cubejs(token, { apiUrl });
}
export async function executeQuery(query: Query) {
@@ -31,6 +21,6 @@ export async function executeQuery(query: Query) {
}
const detail = error instanceof Error && error.message ? ` Details: ${error.message}` : "";
throw new ConfigurationError(`${CUBE_QUERY_ERROR_MESSAGE}${detail}`);
throw new QueryExecutionError(`${CUBE_QUERY_ERROR_MESSAGE}${detail}`);
}
}
@@ -27,6 +27,7 @@ describe("cube-config", () => {
expect(config.apiUrl).toBe("https://cube.formbricks.local/cubejs-api/v1");
expect(jwt.verify(config.token, "cube-secret")).toEqual({
exp: expect.any(Number),
iat: expect.any(Number),
});
});
@@ -6,6 +6,7 @@ import { env } from "@/lib/env";
export const CUBE_CONFIGURATION_ERROR_MESSAGE =
"Cube is not configured on this instance. Set CUBEJS_API_URL and CUBEJS_API_SECRET.";
export const CUBE_API_TOKEN_TTL_SECONDS = 60 * 60;
export const normalizeCubeApiUrl = (baseUrl: string): string => {
if (baseUrl.includes("/cubejs-api/v1")) {
@@ -23,6 +24,9 @@ export const getCubeApiConfig = () => {
return {
apiUrl: normalizeCubeApiUrl(env.CUBEJS_API_URL),
apiSecret: env.CUBEJS_API_SECRET,
token: jwt.sign({}, env.CUBEJS_API_SECRET, { algorithm: "HS256" }),
token: jwt.sign({}, env.CUBEJS_API_SECRET, {
algorithm: "HS256",
expiresIn: CUBE_API_TOKEN_TTL_SECONDS,
}),
};
};
@@ -21,13 +21,17 @@ import { EmptyState } from "@/modules/ui/components/empty-state";
import { GoBackButton } from "@/modules/ui/components/go-back-button";
import { PageContentWrapper } from "@/modules/ui/components/page-content-wrapper";
import { updateDashboardAction, updateWidgetLayoutsAction } from "../actions";
import type { TDashboardWidgetError } from "../lib/widget-errors";
const ROW_HEIGHT = 80;
interface DashboardDetailClientProps {
workspaceId: string;
dashboard: TDashboardDetail;
widgetDataPromises: Map<string, Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: string }>>;
widgetDataPromises: Map<
string,
Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: TDashboardWidgetError }>
>;
directories: { id: string; name: string }[];
isReadOnly: boolean;
}
@@ -96,7 +100,7 @@ const MemoizedWidgetContent = memo(function WidgetContent({
dataPromise,
}: Readonly<{
widget: TDashboardWidget;
dataPromise?: Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: string }>;
dataPromise?: Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: TDashboardWidgetError }>;
}>) {
if (widget.chart && dataPromise) {
return (
@@ -122,7 +126,7 @@ const MemoizedWidgetItem = memo(function WidgetItem({
}: Readonly<{
widget: TDashboardWidget;
isEditing: boolean;
dataPromise?: Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: string }>;
dataPromise?: Promise<{ data: TChartDataRow[]; query: TChartQuery } | { error: TDashboardWidgetError }>;
onEdit?: () => void;
onResize?: () => void;
onRemove?: () => void;
@@ -5,9 +5,10 @@ import { useTranslation } from "react-i18next";
import { TChartQuery } from "@formbricks/types/analysis";
import { ChartRenderer } from "@/modules/ee/analysis/charts/components/chart-renderer";
import type { TChartDataRow, TChartType } from "@/modules/ee/analysis/types/analysis";
import { DASHBOARD_WIDGET_LOAD_ERROR, type TDashboardWidgetError } from "../lib/widget-errors";
interface DashboardWidgetDataProps {
dataPromise: Promise<{ data: TChartDataRow[] } | { error: string }>;
dataPromise: Promise<{ data: TChartDataRow[] } | { error: TDashboardWidgetError }>;
chartType: TChartType;
query: TChartQuery;
}
@@ -17,9 +18,14 @@ export function DashboardWidgetData({ dataPromise, chartType, query }: Readonly<
const result = use(dataPromise);
if ("error" in result) {
const errorMessage =
result.error === DASHBOARD_WIDGET_LOAD_ERROR
? t("workspace.analysis.dashboards.failed_to_load_chart_data")
: t("workspace.analysis.dashboards.failed_to_load_chart_data");
return (
<div className="flex h-full items-center justify-center text-center text-sm text-red-500">
{t("workspace.analysis.dashboards.failed_to_load_chart_data")}: {result.error}
{errorMessage}
</div>
);
}
@@ -0,0 +1,3 @@
export const DASHBOARD_WIDGET_LOAD_ERROR = "failed_to_load_chart_data" as const;
export type TDashboardWidgetError = typeof DASHBOARD_WIDGET_LOAD_ERROR;
@@ -1,4 +1,5 @@
import { notFound } from "next/navigation";
import { logger } from "@formbricks/logger";
import type { TChartQuery } from "@formbricks/types/analysis";
import { ResourceNotFoundError } from "@formbricks/types/errors";
import { executeQuery } from "@/modules/ee/analysis/api/lib/cube-client";
@@ -8,6 +9,7 @@ import { getFeedbackRecordDirectoriesByWorkspaceId } from "@/modules/ee/feedback
import { getWorkspaceAuth } from "@/modules/workspaces/lib/utils";
import { DashboardDetailClient } from "../components/dashboard-detail-client";
import { getDashboard } from "../lib/dashboards";
import { DASHBOARD_WIDGET_LOAD_ERROR, type TDashboardWidgetError } from "../lib/widget-errors";
interface WidgetQueryResult {
data: TChartDataRow[];
@@ -17,18 +19,18 @@ interface WidgetQueryResult {
async function executeWidgetQuery(
query: TChartQuery,
feedbackRecordDirectoryId: string
): Promise<WidgetQueryResult | { error: string }> {
): Promise<WidgetQueryResult | { error: TDashboardWidgetError }> {
try {
const scopedQuery = injectTenantFilter(query, feedbackRecordDirectoryId);
const data = await executeQuery(scopedQuery as Record<string, unknown>);
return { data: Array.isArray(data) ? data : [], query };
} catch (error) {
const message = error instanceof Error ? error.message : "Failed to load chart data";
return { error: message };
logger.error(error, "Failed to load dashboard widget data");
return { error: DASHBOARD_WIDGET_LOAD_ERROR };
}
}
type WidgetQueryPromiseResult = Promise<WidgetQueryResult | { error: string }>;
type WidgetQueryPromiseResult = Promise<WidgetQueryResult | { error: TDashboardWidgetError }>;
export async function DashboardDetailPage({
params,
@@ -49,7 +51,7 @@ export async function DashboardDetailPage({
throw error;
}
const widgetDataPromises = new Map<string, Promise<WidgetQueryResult | { error: string }>>();
const widgetDataPromises = new Map<string, WidgetQueryPromiseResult>();
const widgetsWithCharts = dashboard.widgets.filter(
(w): w is typeof w & { chart: NonNullable<typeof w.chart> } => !!w.chart
);
+2 -2
View File
@@ -18,7 +18,7 @@ cube(`FeedbackRecords`, {
detractorCount: {
type: `count`,
filters: [{ sql: `${CUBE}.value_number <= 6` }],
filters: [{ sql: `${CUBE}.value_number >= 0 AND ${CUBE}.value_number <= 6` }],
description: `Number of detractors (NPS score 0-6)`,
},
@@ -146,7 +146,7 @@ cube(`TopicsUnnested`, {
dimensions: {
id: {
sql: `feedback_record_id || '-' || topic`,
sql: `md5(feedback_record_id || '::' || topic)`,
type: `string`,
primaryKey: true,
},
+10
View File
@@ -39,6 +39,14 @@ class ConfigurationError extends Error {
}
}
class QueryExecutionError extends Error {
statusCode = 500;
constructor(message: string) {
super(message);
this.name = "QueryExecutionError";
}
}
class UnknownError extends Error {
statusCode = 500;
constructor(message: string) {
@@ -144,6 +152,7 @@ export {
InvalidInputError,
ValidationError,
ConfigurationError,
QueryExecutionError,
DatabaseError,
UniqueConstraintError,
UnknownError,
@@ -166,6 +175,7 @@ export const EXPECTED_ERROR_NAMES = new Set([
"InvalidInputError",
"ValidationError",
"ConfigurationError",
"QueryExecutionError",
"AuthenticationError",
"OperationNotAllowedError",
"TooManyRequestsError",