From 93f49f03db7c635bf26dfc1c1c0faa8d603f3f5b Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 29 Sep 2025 08:52:51 -0600 Subject: [PATCH 01/14] Fix: Bedrock caches differently than anthropic --- packages/ai/src/llm/providers/gateway.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ai/src/llm/providers/gateway.ts b/packages/ai/src/llm/providers/gateway.ts index 3198fc468..0d6f834c7 100644 --- a/packages/ai/src/llm/providers/gateway.ts +++ b/packages/ai/src/llm/providers/gateway.ts @@ -10,10 +10,10 @@ export const DEFAULT_ANTHROPIC_OPTIONS = { cacheControl: { type: 'ephemeral' }, }, bedrock: { - cacheControl: { type: 'ephemeral' }, - additionalModelRequestFields: { - anthropic_beta: ['fine-grained-tool-streaming-2025-05-14'], - }, + cachePoint: { type: 'default' } + }, + additionalModelRequestFields: { + anthropic_beta: ['fine-grained-tool-streaming-2025-05-14'], }, }; From c603ae1ebf365df4e98a62e247f770268139cfdc Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 29 Sep 2025 09:38:49 -0600 Subject: [PATCH 02/14] move additional fields back in. --- packages/ai/src/llm/providers/gateway.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ai/src/llm/providers/gateway.ts b/packages/ai/src/llm/providers/gateway.ts index 0d6f834c7..50df61eaa 100644 --- a/packages/ai/src/llm/providers/gateway.ts +++ b/packages/ai/src/llm/providers/gateway.ts @@ -10,10 +10,10 @@ export const DEFAULT_ANTHROPIC_OPTIONS = { cacheControl: { type: 'ephemeral' }, }, bedrock: { - cachePoint: { type: 'default' } - }, - additionalModelRequestFields: { - anthropic_beta: ['fine-grained-tool-streaming-2025-05-14'], + cachePoint: { type: 'default' }, + additionalModelRequestFields: { + anthropic_beta: ['fine-grained-tool-streaming-2025-05-14'], + }, }, }; From 200ea567a7bf8075e4d5a12810fefa80f9861b51 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 29 Sep 2025 09:46:31 -0600 Subject: [PATCH 03/14] add in type for anthropic gateway --- packages/ai/src/llm/providers/gateway.ts | 41 ++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/packages/ai/src/llm/providers/gateway.ts b/packages/ai/src/llm/providers/gateway.ts index 50df61eaa..c5438b7c8 100644 --- a/packages/ai/src/llm/providers/gateway.ts +++ b/packages/ai/src/llm/providers/gateway.ts @@ -2,7 +2,44 @@ import { createGateway } from '@ai-sdk/gateway'; import { wrapLanguageModel } from 'ai'; import { BraintrustMiddleware } from 'braintrust'; -export const DEFAULT_ANTHROPIC_OPTIONS = { +// Provider-specific option types +export type GatewayProviderOrder = string[]; + +export type AnthropicOptions = { + cacheControl?: { type: 'ephemeral' }; +}; + +export type BedrockOptions = { + cachePoint?: { type: 'default' }; + additionalModelRequestFields?: { + anthropic_beta?: string[]; + }; +}; + +export type OpenAIOptions = { + // parallelToolCalls?: boolean; + reasoningEffort?: 'low' | 'medium' | 'high' | 'minimal'; + verbosity?: 'low' | 'medium' | 'high'; +}; + +// Main provider options types +export type AnthropicProviderOptions = { + gateway: { + order: GatewayProviderOrder; + }; + anthropic: AnthropicOptions; + bedrock: BedrockOptions; +}; + +export type OpenAIProviderOptions = { + gateway: { + order: GatewayProviderOrder; + }; + openai: OpenAIOptions; +}; + +// Default options with proper typing +export const DEFAULT_ANTHROPIC_OPTIONS: AnthropicProviderOptions = { gateway: { order: ['bedrock', 'anthropic', 'vertex'], }, @@ -17,7 +54,7 @@ export const DEFAULT_ANTHROPIC_OPTIONS = { }, }; -export const DEFAULT_OPENAI_OPTIONS = { +export const DEFAULT_OPENAI_OPTIONS: OpenAIProviderOptions = { gateway: { order: ['openai'], }, From 68a5a4f8747ca6e2db1d1ae99743a0d71f819f15 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 29 Sep 2025 10:22:23 -0600 Subject: [PATCH 04/14] i-am-playing-wack-a-mole-rn --- .../analyst-agent/analyst-agent-prompt.txt | 55 +++++++++++-------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt index b94b63484..b9dba77cb 100644 --- a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt +++ b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt @@ -420,28 +420,31 @@ You operate in a loop to complete tasks: - Number cards should always have a metricHeader and metricSubheader. - Always use your best judgment when selecting visualization types, and be confident in your decision - When building horizontal bar charts, put your desired x-axis as the y and the desired y-axis as the x in chartConfig (e.g. if i want my y-axis to be the product name and my x-axis to be the revenue, in my chartConfig i would do barAndLineAxis: x: [product_name] y: [revenue] and allow the front end to handle the horizontal orientation) -- **Using "category" vs. "colorBy" Guidelines** - - When adding dimensions to a bar or line chart, carefully decide whether to use `category` or `colorBy`. - - **Use `category` when you want to create separate series or groups of values.** - - Each category value generates its own line, bar group, or stacked section. - - Examples: - - Line chart with revenue by month split by region → `category: [region]` - - Grouped bar chart of sales by product split by channel → `category: [channel]` - - Rule of thumb: use `category` when the visualization should **separate and compare multiple data series**. - - **Use `colorBy` when you want to keep a single series (no grouping, stacking, etc) but visually differentiate elements by color.** - - Bars or lines remain part of one series, but the colors vary by the field. - - Examples: - - Bar chart of sales by sales rep, with bars colored by region → `colorBy: [region]` - - Line chart of monthly revenue, with points/segments colored by product line → `colorBy: [product_line]` - - Rule of thumb: use `colorBy` when the visualization should **highlight categories inside a single series** rather than split into multiple groups. - - **Guidance by chart type**: - - Bar/Line: - - `category` → multiple series (grouped/stacked bars, multiple lines). - - `colorBy` → single series, colored by attribute. - - **Quick heuristic**: - - Ask: “Does the user want multiple grouped/stacked series, or just one series with colored differentiation?” - - Multiple → `category` - - One series, just colored → `colorBy` +- Using a category or grouping as a "category" vs. "colorBy" + - Category = split the chart into multiple series. Think “make separate mini-charts inside one chart”—one line or bar-group per value. Use when the chart’s purpose is to **compare groups as distinct series**. + - ColorBy = shade within a single series. Think “paint the bars/line points in one series by an attribute.” Use when the chart’s purpose is to **stay a single series** but visually differentiate items by a secondary attribute (color). + - Series decision rule (critical) + - If there is **one quantitative measure** on Y (e.g., `ytd_sales`) and the chart is broken down by a **primary dimension** (e.g., `sales_rep_name`), the default is **one series** → keep `category` **empty** and, if needed, apply `colorBy: []`. + - Only use `category: []` when you truly intend **multiple series** (e.g., multiple lines over time, grouped/stacked bars per group). + - Operational rules + - Single-series visualization: `category: []`, optional `colorBy: ['']` to differentiate. + Example: *Revenue by customer, colored by region* → one bar per rep, `colorBy: ['region']`. + - Multi-series visualization: `category: ['']`, `colorBy: []`. + Example: *Revenue trend by month, split by region* → multiple lines, `category: ['region']`. + - Mutual exclusivity (default): Set **either** `category` **or** `colorBy`, **not both**. The only exception is when the requirement explicitly needs both (rare). If both are set, document why in the metric description. + - Common misunderstandings to avoid + - Do **not** upgrade a single-series chart into grouped/stacked multiple series just because a secondary field exists. If the primary ask is “performance by rep,” that remains **one series**; use `colorBy` for secondary context (e.g., region, role, status). + - “Comparison” language in a title/description does **not** automatically mean multi-series. If the core breakdown is still the *same single unit list* (e.g., reps), stay single-series and use `colorBy`. + - Concrete mapping + - Split/group by X → `category: ['X']` (multiple series). + - Color/highlight by X → `colorBy: ['X']` (single series). + - Safeguard example (single-series with color vs multi-series) + - Wrong: Sales rep bars split into East/West groups (`category: ['region']`) → creates multiple bars per rep, obscuring the rep-level comparison. + - Correct: One bar per rep, colored by East/West (`colorBy: ['region']`) → preserves single-series intent and adds clear context with color. + - Reason: The primary breakdown is the rep list (single series). Region is secondary context, so it belongs in `colorBy`. + - Titles and descriptions should not force multi-series + - Words like “comparison” or “versus” in titles/descriptions do not require multi-series. If the core breakdown is still a single list (e.g., sales reps), keep a single series and use `colorBy` for contextual differences. + - If a single-series chart without color is sufficient, **do not** add `category` or `colorBy`. ... - Visualization Design Guidelines - Always display names instead of IDs when available (e.g., "Product Name" instead of "Product ID") @@ -493,6 +496,9 @@ You operate in a loop to complete tasks: - For monthly trends across years: barAndLineAxis: { x: [month, year], y: [...] } - For quarterly trends: barAndLineAxis: { x: [quarter, year], y: [...] } - For single-year monthly trends: x: [month] (labels render as January, February, …) +- Category Check + - If `barAndLineAxis.x` or `comboChartAxis.x` contains a single non-time dimension (e.g., a list of entities like reps or products), and `y` contains a single metric, default to **single series**: `category: []`. Use `colorBy` for any secondary attribute if needed. + - If `x` is a **time axis** and the requirement is to compare groups **as separate series** over time, then use `category: ['']`. @@ -508,8 +514,9 @@ You operate in a loop to complete tasks: - The system is read-only and you cannot write to databases. - Only the following chart types are supported: table, line, bar, combo, pie/donut, number cards, and scatter plot. Other chart types are not supported. - You cannot write Python. -- You cannot highlight or flag specific elements (e.g., lines, bars, cells) within visualizations; it can only control the general color theme. -- You cannot attach specific colors to specific elements within visualizations. Only general color themes are supported. +- You cannot assign custom hex colors to specific individual elements (e.g., hard-map “East = #123456”). Only default palettes are supported. +- You cannot “spot highlight” arbitrary single bars/points by ID. + - **`colorBy` is supported** and should be used to apply the default palette to a **single series** based on a categorical field (e.g., color bars by `region` without creating multiple series). - Individual metrics cannot include additional descriptions, assumptions, or commentary. - Dashboard layout constraints: - Dashboards display collections of existing metrics referenced by their IDs. From ebf10a9881c98415bfa9ab023ee4579c0e40d708 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 29 Sep 2025 11:00:20 -0600 Subject: [PATCH 05/14] more-colorby-fix-attempts --- .../analyst-agent/analyst-agent-prompt.txt | 61 +++++++++++-------- .../helpers/metric-tool-description.txt | 51 +--------------- 2 files changed, 38 insertions(+), 74 deletions(-) diff --git a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt index b9dba77cb..0add6d03a 100644 --- a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt +++ b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt @@ -420,31 +420,42 @@ You operate in a loop to complete tasks: - Number cards should always have a metricHeader and metricSubheader. - Always use your best judgment when selecting visualization types, and be confident in your decision - When building horizontal bar charts, put your desired x-axis as the y and the desired y-axis as the x in chartConfig (e.g. if i want my y-axis to be the product name and my x-axis to be the revenue, in my chartConfig i would do barAndLineAxis: x: [product_name] y: [revenue] and allow the front end to handle the horizontal orientation) -- Using a category or grouping as a "category" vs. "colorBy" - - Category = split the chart into multiple series. Think “make separate mini-charts inside one chart”—one line or bar-group per value. Use when the chart’s purpose is to **compare groups as distinct series**. - - ColorBy = shade within a single series. Think “paint the bars/line points in one series by an attribute.” Use when the chart’s purpose is to **stay a single series** but visually differentiate items by a secondary attribute (color). - - Series decision rule (critical) - - If there is **one quantitative measure** on Y (e.g., `ytd_sales`) and the chart is broken down by a **primary dimension** (e.g., `sales_rep_name`), the default is **one series** → keep `category` **empty** and, if needed, apply `colorBy: []`. - - Only use `category: []` when you truly intend **multiple series** (e.g., multiple lines over time, grouped/stacked bars per group). - - Operational rules - - Single-series visualization: `category: []`, optional `colorBy: ['']` to differentiate. - Example: *Revenue by customer, colored by region* → one bar per rep, `colorBy: ['region']`. - - Multi-series visualization: `category: ['']`, `colorBy: []`. - Example: *Revenue trend by month, split by region* → multiple lines, `category: ['region']`. - - Mutual exclusivity (default): Set **either** `category` **or** `colorBy`, **not both**. The only exception is when the requirement explicitly needs both (rare). If both are set, document why in the metric description. - - Common misunderstandings to avoid - - Do **not** upgrade a single-series chart into grouped/stacked multiple series just because a secondary field exists. If the primary ask is “performance by rep,” that remains **one series**; use `colorBy` for secondary context (e.g., region, role, status). - - “Comparison” language in a title/description does **not** automatically mean multi-series. If the core breakdown is still the *same single unit list* (e.g., reps), stay single-series and use `colorBy`. - - Concrete mapping - - Split/group by X → `category: ['X']` (multiple series). - - Color/highlight by X → `colorBy: ['X']` (single series). - - Safeguard example (single-series with color vs multi-series) - - Wrong: Sales rep bars split into East/West groups (`category: ['region']`) → creates multiple bars per rep, obscuring the rep-level comparison. - - Correct: One bar per rep, colored by East/West (`colorBy: ['region']`) → preserves single-series intent and adds clear context with color. - - Reason: The primary breakdown is the rep list (single series). Region is secondary context, so it belongs in `colorBy`. - - Titles and descriptions should not force multi-series - - Words like “comparison” or “versus” in titles/descriptions do not require multi-series. If the core breakdown is still a single list (e.g., sales reps), keep a single series and use `colorBy` for contextual differences. - - If a single-series chart without color is sufficient, **do not** add `category` or `colorBy`. +- Using a categorical field as "category" vs. "colorBy" + - Critical Clarification: + - Many fields are categorical (text, labels, enums), but this does **not** mean they belong in `category`. + - `category` in chartConfig has a very specific meaning: *split into multiple parallel series*. + - Most categorical-looking fields should instead be used in `colorBy`, not in `category`. + - Decision Rule + - Start by asking: *Is this field defining the primary series, or just adding context?* + - Primary series split → use category + - Example: *Revenue over time by region* → multiple lines (category = region). + - Context only → use colorBy + - Example: *Sales reps’ performance colored by region* → one bar per rep (colorBy = region). + - Checklist Before Using category + 1. Is the X-axis a time series and the user wants multiple lines/bars? → `category`. + 2. Does the chart need parallel groups of the same measure (e.g., stacked/grouped bars)? → `category`. + 3. Otherwise (entity list on X, one measure on Y) → keep single series, use `colorBy` if needed. + - Common Confusion Traps + - Fields like `region`, `segment`, `department`, `status`, `role`, etc. *look categorical* but usually belong in `colorBy` when the main X is an entity list (e.g., reps, products). + - Do **not** force `category` just because the field name sounds like a grouping. + - Example: *Compare East vs West reps* → Wrong = `category = region` (duplicates reps). Correct = `colorBy = region`. + - Examples + - Correct — category + - "Show monthly revenue by region" → X = month, Y = revenue, `category = region` → multiple lines. + - "Stacked bars of sales by product category" → X = category, Y = sales, `category = product_type`. + - Correct — colorBy + - "Compare invdividual customers and their revenue from East vs West" → X = rep, Y = sales, `colorBy = region` → one bar per rep, East as one color/West as another color. + - "Quota attainment by department" → X = rep, Y = quota %, `colorBy = department`. + - Incorrect — misuse of category + - Wrong: "Compare East vs West reps" → X = rep, Y = sales, `category = region` (creates duplicate reps, confusing grouped bars). + - Wrong: "Product sales by category" when only one measure → `category = product_type` (splits into parallel series unnecessarily). + - Safeguards + - Do **not** automatically map categorical fields to `category`. + - Use **either** `category` or `colorBy`, never both. + - “Comparison” or “versus” in user wording does not imply multiple series. If the main breakdown is still a single list, use `colorBy`. + - Rule of Thumb for Categorical Fields + - *If the request is centered on comparing items in a single list → use colorBy.* + - *If the request is centered on comparing groups as separate series → use category.* ... - Visualization Design Guidelines - Always display names instead of IDs when available (e.g., "Product Name" instead of "Product ID") diff --git a/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt b/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt index 12ab80021..56f91f9f8 100644 --- a/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt +++ b/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt @@ -547,30 +547,12 @@ definitions: type: array items: type: string - description: | - LOWERCASE column name from SQL for category grouping. - - Category vs ColorBy: - - Use CATEGORY: When you want to create multiple series/lines (one per category value). Creates separate data points and enables legend. - - Use COLORBY: When you want to apply colors to bars/points based on a column value, but keep them as a single series. - - Example: Sales by month - - With category=['region']: Creates separate lines/bars for each region (North, South, East, West) - - With colorBy=['region']: Colors bars by region but keeps them as one series + description: LOWERCASE column name from SQL for category grouping. tooltip: type: [array, 'null'] items: type: string description: Column names for tooltip. If null, y axis is used. Default: null - colorBy: - type: [array, 'null'] - items: - type: string - description: | - Optional array of column names to apply colors based on column values. - Use this when you want visual differentiation without creating separate series. - Perfect for: Status indicators (red/yellow/green), priority levels, or any categorical color coding. - Example: ['region'] - colors bars by region values required: - x - y @@ -626,9 +608,6 @@ definitions: type: array items: type: string - description: | - LOWERCASE column name from SQL for category grouping. - Creates separate colored series of points for each category value. size: type: array items: @@ -639,14 +618,6 @@ definitions: items: type: string description: Column names for tooltip. If null, y axis is used. Default: null - colorBy: - type: [array, 'null'] - items: - type: string - description: | - Optional array of column names to apply colors to scatter points based on column values. - Use when you want all points in one series with color-coded differentiation. - Example: ['priority'] - colors points by priority values required: - x - y @@ -678,14 +649,6 @@ definitions: items: type: string description: Column names for tooltip. If null, y axis is used. Default: null - colorBy: - type: [array, 'null'] - items: - type: string - description: | - Optional array of column names for pie slice colors based on column values. - Allows custom color mapping for specific slice categories. - Example: ['category'] - colors slices by category values required: - x - y @@ -720,22 +683,12 @@ definitions: type: array items: type: string - description: | - LOWERCASE column name from SQL for category grouping. - Creates separate series for each category value in combo chart. + description: LOWERCASE column name from SQL for category grouping. tooltip: type: [array, 'null'] items: type: string description: Column names for tooltip. If null, y axis is used. Default: null - colorBy: - type: [array, 'null'] - items: - type: string - description: | - Optional array of column names to apply colors based on column values in combo chart. - Useful for color-coding bars while lines remain as separate series. - Example: ['region'] - colors bars by region values required: - x - y From 09030d5d2bea90e158c639692336000ac9918c52 Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Mon, 29 Sep 2025 11:02:56 -0600 Subject: [PATCH 06/14] migrating /chats/[id] GET endpoint to v2 --- apps/server/src/api/v2/chats/[id]/GET.ts | 87 +++++++++++++ apps/server/src/api/v2/chats/[id]/index.ts | 8 ++ apps/server/src/api/v2/chats/index.ts | 2 + .../src/api/v2/chats/services/chat-helpers.ts | 117 +++++++++++------- .../v2/chats/services/chat-service.test.ts | 51 +++++--- .../src/api/v2/dashboards/[id]/GET.test.ts | 8 +- apps/server/src/api/v2/dashboards/[id]/GET.ts | 4 +- .../src/shared-helpers/metric-helpers.test.ts | 14 +-- .../src/shared-helpers/metric-helpers.ts | 4 +- .../web/src/api/buster_rest/chats/requests.ts | 2 +- .../src/assets/cascading-permissions.ts | 102 ++++++++++++++- packages/access-controls/src/assets/checks.ts | 2 - .../get-users-with-asset-permissions.ts | 60 +++++++++ packages/database/src/queries/assets/index.ts | 7 ++ .../check-chats-containing-asset.ts | 2 +- ...-users-with-dashboard-permissions-by-id.ts | 58 --------- .../database/src/queries/dashboards/index.ts | 7 -- .../database/src/queries/messages/messages.ts | 16 ++- ...get-users-with-metric-permissions-by-id.ts | 58 --------- .../database/src/queries/metrics/index.ts | 8 -- .../src/chats/chat-message.types.ts | 2 + 21 files changed, 403 insertions(+), 216 deletions(-) create mode 100644 apps/server/src/api/v2/chats/[id]/GET.ts create mode 100644 apps/server/src/api/v2/chats/[id]/index.ts create mode 100644 packages/database/src/queries/assets/get-users-with-asset-permissions.ts delete mode 100644 packages/database/src/queries/dashboards/get-users-with-dashboard-permissions-by-id.ts delete mode 100644 packages/database/src/queries/metrics/get-users-with-metric-permissions-by-id.ts diff --git a/apps/server/src/api/v2/chats/[id]/GET.ts b/apps/server/src/api/v2/chats/[id]/GET.ts new file mode 100644 index 000000000..d95547741 --- /dev/null +++ b/apps/server/src/api/v2/chats/[id]/GET.ts @@ -0,0 +1,87 @@ +import { checkPermission } from '@buster/access-controls'; +import type { User } from '@buster/database/queries'; +import { + getChatWithDetails, + getMessagesForChatWithUserDetails, +} from '@buster/database/queries'; +import { + type GetChatResponse, + GetChatRequestSchema, +} from '@buster/server-shared/chats'; +import { zValidator } from '@hono/zod-validator'; +import { Hono } from 'hono'; +import { HTTPException } from 'hono/http-exception'; +import { throwUnauthorizedError } from '../../../../shared-helpers/asset-public-access'; +import { buildChatWithMessages } from '../services/chat-helpers'; + +interface GetChatHandlerParams { + chatId: string; + user: User; +} + +const app = new Hono().get('/', zValidator('param', GetChatRequestSchema), async (c) => { + const { id } = c.req.valid('param'); + const user = c.get('busterUser'); + + console.info(`Processing GET request for chat with ID: ${id}, user_id: ${user.id}`); + + const response: GetChatResponse = await getChatHandler({ chatId: id, user }); + + return c.json(response); +}); + +export default app; + +/** + * Handler to retrieve a chat by ID with messages and permissions + * This is the TypeScript equivalent of the Rust get_chat_handler + */ +export async function getChatHandler(params: GetChatHandlerParams): Promise { + const { chatId, user } = params; + + // Fetch chat with messages and related data + const chatData = await getChatWithDetails({ + chatId, + userId: user.id, + }); + + if (!chatData) { + console.warn(`Chat not found: ${chatId}`); + throw new HTTPException(404, { + message: 'Chat not found', + }); + } + + const { chat, user: creator } = chatData; + console.info('user', user); + + // Check permissions using the access control system + const { hasAccess, effectiveRole } = await checkPermission({ + userId: user.id, + assetId: chatId, + assetType: 'chat', + requiredRole: 'can_view', + organizationId: chat.organizationId, + workspaceSharing: chat.workspaceSharing || 'none', + publiclyAccessible: chat.publiclyAccessible || false, + publicExpiryDate: chat.publicExpiryDate ?? undefined, + }); + + if (!hasAccess || !effectiveRole) { + throwUnauthorizedError({ + publiclyAccessible: chat.publiclyAccessible || false, + publicExpiryDate: chat.publicExpiryDate ?? undefined, + }); + } + + const messages = await getMessagesForChatWithUserDetails(chatId); + + const response: GetChatResponse = await buildChatWithMessages( + chat, + messages, + creator, + effectiveRole + ); + + return response; +} diff --git a/apps/server/src/api/v2/chats/[id]/index.ts b/apps/server/src/api/v2/chats/[id]/index.ts new file mode 100644 index 000000000..5f75e86fe --- /dev/null +++ b/apps/server/src/api/v2/chats/[id]/index.ts @@ -0,0 +1,8 @@ +import { Hono } from 'hono'; +import GET from './GET'; + +const app = new Hono(); + +app.route('/', GET); + +export default app; diff --git a/apps/server/src/api/v2/chats/index.ts b/apps/server/src/api/v2/chats/index.ts index 716744e94..37c44ca8c 100644 --- a/apps/server/src/api/v2/chats/index.ts +++ b/apps/server/src/api/v2/chats/index.ts @@ -12,6 +12,7 @@ import '../../../types/hono.types'; //I added this to fix intermitent type error import { HTTPException } from 'hono/http-exception'; import { z } from 'zod'; import GET from './GET'; +import chatById from './[id]'; import { cancelChatHandler } from './cancel-chat'; import { createChatHandler } from './handler'; @@ -19,6 +20,7 @@ const app = new Hono() // Apply authentication middleware .use('*', requireAuth) .route('/', GET) + .route('/:id', chatById) // POST /chats - Create a new chat .post('/', zValidator('json', ChatCreateRequestSchema), async (c) => { const request = c.req.valid('json'); diff --git a/apps/server/src/api/v2/chats/services/chat-helpers.ts b/apps/server/src/api/v2/chats/services/chat-helpers.ts index adc8fc0b0..519e1781f 100644 --- a/apps/server/src/api/v2/chats/services/chat-helpers.ts +++ b/apps/server/src/api/v2/chats/services/chat-helpers.ts @@ -1,4 +1,8 @@ -import { canUserAccessChatCached } from '@buster/access-controls'; +import { + type AssetPermissionRole, + canUserAccessChatCached, + checkPermission, +} from '@buster/access-controls'; import type { ModelMessage } from '@buster/ai'; import { db } from '@buster/database/connection'; import { @@ -7,7 +11,9 @@ import { createMessage, generateAssetMessages, getChatWithDetails, - getMessagesForChat, + getMessagesForChatWithUserDetails, + getOrganizationMemberCount, + getUsersWithAssetPermissions, } from '@buster/database/queries'; import type { Chat, Message } from '@buster/database/queries'; import { chats, messages } from '@buster/database/schema'; @@ -24,6 +30,8 @@ import { ChatError, ChatErrorCode } from '@buster/server-shared/chats'; import { PostProcessingMessageSchema } from '@buster/server-shared/message'; import { and, eq, gte, isNull } from 'drizzle-orm'; import type { z } from 'zod'; +import { throwUnauthorizedError } from '../../../../shared-helpers/asset-public-access'; +import { getPubliclyEnabledByUser } from '../../../../shared-helpers/get-publicly-enabled-by-user'; /** * Validates a nullable JSONB field against a Zod schema @@ -106,64 +114,72 @@ const buildReasoningMessages = (reasoningMessages: unknown): ChatMessage['reason * Build a ChatWithMessages object from database entities * Optimized for performance with pre-allocated objects and minimal iterations */ -export function buildChatWithMessages( +export async function buildChatWithMessages( chat: Chat, - messages: Message[], + messages: { message: Message; user: User }[], user: User | null, + permission: AssetPermissionRole, isFavorited = false -): ChatWithMessages { +): Promise { + const createdByName = user?.name || user?.email || 'Unknown User'; + // Pre-allocate collections with known size const messageCount = messages.length; const messageMap: Record = {}; const messageIds: string[] = new Array(messageCount); - // Cache user info to avoid repeated property access - const userName = user?.name || user?.email || 'Unknown User'; - const userAvatar = user?.avatarUrl || undefined; - // Single iteration with optimized object creation for (let i = 0; i < messageCount; i++) { const msg = messages[i]; if (!msg) continue; // Skip if somehow undefined - const responseMessages = buildResponseMessages(msg.responseMessages); - const reasoningMessages = buildReasoningMessages(msg.reasoning); + const responseMessages = buildResponseMessages(msg.message.responseMessages); + const reasoningMessages = buildReasoningMessages(msg.message.reasoning); // Pre-compute arrays to avoid Object.keys() calls const responseMessageIds = Object.keys(responseMessages); const reasoningMessageIds = Object.keys(reasoningMessages); - const requestMessage = msg.requestMessage + const requestMessage = msg.message.requestMessage ? { - request: msg.requestMessage, - sender_id: msg.createdBy, - sender_name: userName, - sender_avatar: userAvatar, + request: msg.message.requestMessage, + sender_id: msg.message.createdBy, + sender_name: msg.user.name || msg.user.email || 'Unknown User', + sender_avatar: msg.user.avatarUrl, } : null; const chatMessage: ChatMessage = { - id: msg.id, - created_at: msg.createdAt, - updated_at: msg.updatedAt, + id: msg.message.id, + created_at: msg.message.createdAt, + updated_at: msg.message.updatedAt, request_message: requestMessage, response_messages: responseMessages, response_message_ids: responseMessageIds, reasoning_message_ids: reasoningMessageIds, reasoning_messages: reasoningMessages, - final_reasoning_message: msg.finalReasoningMessage || null, - feedback: msg.feedback ? (msg.feedback as 'negative') : null, - is_completed: msg.isCompleted || false, + final_reasoning_message: msg.message.finalReasoningMessage || null, + feedback: msg.message.feedback ? (msg.message.feedback as 'negative') : null, + is_completed: msg.message.isCompleted || false, post_processing_message: validateNullableJsonb( - msg.postProcessingMessage, + msg.message.postProcessingMessage, PostProcessingMessageSchema ), }; - messageIds[i] = msg.id; - messageMap[msg.id] = chatMessage; + messageIds[i] = msg.message.id; + messageMap[msg.message.id] = chatMessage; } + const [publiclyEnabledBy, individualPermissions, workspaceMemberCount] = await Promise.all([ + getPubliclyEnabledByUser(chat.publiclyEnabledBy), + getUsersWithAssetPermissions({ + assetId: chat.id, + assetType: 'chat', + }), + getOrganizationMemberCount(chat.organizationId), + ]); + // Ensure message_ids array has no duplicates const uniqueMessageIds = [...new Set(messageIds)]; @@ -181,17 +197,16 @@ export function buildChatWithMessages( updated_at: chat.updatedAt, created_by: chat.createdBy, created_by_id: chat.createdBy, - created_by_name: userName, + created_by_name: createdByName, created_by_avatar: user?.avatarUrl || null, - // Sharing fields - TODO: implement proper sharing logic - individual_permissions: [], + individual_permissions: individualPermissions, publicly_accessible: chat.publiclyAccessible || false, public_expiry_date: chat.publicExpiryDate || null, - public_enabled_by: chat.publiclyEnabledBy || null, - public_password: null, // Don't expose password - permission: 'owner', // TODO: Implement proper permission checking - workspace_sharing: 'full_access', - workspace_member_count: 0, + public_enabled_by: publiclyEnabledBy, + public_password: null, // password not implemented yet + permission, + workspace_sharing: chat.workspaceSharing || 'none', + workspace_member_count: workspaceMemberCount, }; } @@ -221,16 +236,22 @@ export async function handleExistingChat( throw new ChatError(ChatErrorCode.CHAT_NOT_FOUND, 'Chat not found', 404); } - const hasPermission = await canUserAccessChatCached({ + const { effectiveRole, hasAccess } = await checkPermission({ userId: user.id, - chatId, + assetId: chatId, + assetType: 'chat', + requiredRole: 'can_view', + organizationId: chatDetails.chat.organizationId, + workspaceSharing: chatDetails.chat.workspaceSharing, + publiclyAccessible: chatDetails.chat.publiclyAccessible, + publicExpiryDate: chatDetails.chat.publicExpiryDate || undefined, }); - if (!hasPermission) { - throw new ChatError( - ChatErrorCode.PERMISSION_DENIED, - 'You do not have permission to access this chat', - 403 - ); + + if (!hasAccess || !effectiveRole) { + throwUnauthorizedError({ + publiclyAccessible: chatDetails.chat.publiclyAccessible, + publicExpiryDate: chatDetails.chat.publicExpiryDate || undefined, + }); } // Handle redo logic if redoFromMessageId is provided @@ -266,17 +287,20 @@ export async function handleExistingChat( metadata, }) : Promise.resolve(null), - getMessagesForChat(chatId), + getMessagesForChatWithUserDetails(chatId), ]); // Combine messages - prepend new message to maintain descending order (newest first) - const allMessages = newMessage ? [newMessage, ...existingMessages] : existingMessages; + const allMessages = newMessage + ? [{ message: newMessage, user }, ...existingMessages] + : existingMessages; // Build chat with messages - const chatWithMessages: ChatWithMessages = buildChatWithMessages( + const chatWithMessages: ChatWithMessages = await buildChatWithMessages( chatDetails.chat, allMessages, chatDetails.user, + effectiveRole, chatDetails.isFavorited ); @@ -378,10 +402,11 @@ export async function handleNewChat({ }); // Build chat with messages - const chatWithMessages = buildChatWithMessages( + const chatWithMessages = await buildChatWithMessages( result.chat, - result.message ? [result.message] : [], + result.message ? [{ message: result.message, user }] : [], user, + 'owner', false ); diff --git a/apps/server/src/api/v2/chats/services/chat-service.test.ts b/apps/server/src/api/v2/chats/services/chat-service.test.ts index 0874e9531..51d8351d1 100644 --- a/apps/server/src/api/v2/chats/services/chat-service.test.ts +++ b/apps/server/src/api/v2/chats/services/chat-service.test.ts @@ -3,14 +3,15 @@ import { ChatError, ChatErrorCode } from '@buster/server-shared/chats'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { initializeChat } from './chat-service'; -import { canUserAccessChatCached } from '@buster/access-controls'; +import { canUserAccessChatCached, checkPermission } from '@buster/access-controls'; // Import mocked functions import { - createChat, createMessage, - generateAssetMessages, getChatWithDetails, getMessagesForChat, + getMessagesForChatWithUserDetails, + getOrganizationMemberCount, + getUsersWithAssetPermissions, } from '@buster/database/queries'; const mockUser = { @@ -90,18 +91,34 @@ vi.mock('@buster/database/queries', () => ({ getChatWithDetails: vi.fn(), createMessage: vi.fn(), generateAssetMessages: vi.fn(), - getMessagesForChat: vi.fn(), + getMessagesForChatWithUserDetails: vi.fn(), createAssetPermission: vi.fn(), + getUsersWithAssetPermissions: vi.fn(), + getOrganizationMemberCount: vi.fn(), })); // Mock access-controls vi.mock('@buster/access-controls', () => ({ - canUserAccessChatCached: vi.fn(), + checkPermission: vi.fn(), })); describe('chat-service', () => { + const mockCheckPermission = checkPermission as any; + const mockGetOrganizationMemberCount = getOrganizationMemberCount as any; + const mockGetUsersWithAssetPermissions = getUsersWithAssetPermissions as any; + const mockGetMessagesForChatWithUserDetails = getMessagesForChatWithUserDetails as any; + beforeEach(() => { vi.clearAllMocks(); + + // Setup default mock returns + mockCheckPermission.mockResolvedValue({ + hasAccess: true, + effectiveRole: 'can_view', + }); + mockGetOrganizationMemberCount.mockResolvedValue(5); + mockGetUsersWithAssetPermissions.mockResolvedValue([]); + mockGetMessagesForChatWithUserDetails.mockResolvedValue([]); }); describe('initializeChat', () => { @@ -136,9 +153,15 @@ describe('chat-service', () => { 'org-123' ); - expect(canUserAccessChatCached).toHaveBeenCalledWith({ + expect(mockCheckPermission).toHaveBeenCalledWith({ userId: mockUser.id, - chatId: 'chat-123', + assetId: 'chat-123', + assetType: 'chat', + requiredRole: 'can_view', + organizationId: '550e8400-e29b-41d4-a716-446655440000', + publiclyAccessible: false, + publicExpiryDate: undefined, + workspaceSharing: undefined, }); expect(createMessage).toHaveBeenCalledWith({ chatId: 'chat-123', @@ -150,7 +173,10 @@ describe('chat-service', () => { }); it('should throw PERMISSION_DENIED error when user lacks permission', async () => { - vi.mocked(canUserAccessChatCached).mockResolvedValue(false); + mockCheckPermission.mockResolvedValue({ + hasAccess: false, + effectiveRole: null, + }); vi.mocked(getChatWithDetails).mockResolvedValue({ chat: mockChat, user: { id: 'user-123', name: 'Test User', avatarUrl: null } as any, @@ -159,14 +185,7 @@ describe('chat-service', () => { await expect( initializeChat({ chat_id: 'chat-123', prompt: 'Hello' }, mockUser, 'org-123') - ).rejects.toThrow(ChatError); - - await expect( - initializeChat({ chat_id: 'chat-123', prompt: 'Hello' }, mockUser, 'org-123') - ).rejects.toMatchObject({ - code: ChatErrorCode.PERMISSION_DENIED, - statusCode: 403, - }); + ).rejects.toThrow('You do not have permission to access this asset'); }); it('should throw CHAT_NOT_FOUND error when chat does not exist', async () => { diff --git a/apps/server/src/api/v2/dashboards/[id]/GET.test.ts b/apps/server/src/api/v2/dashboards/[id]/GET.test.ts index 90bb13198..8eb121f16 100644 --- a/apps/server/src/api/v2/dashboards/[id]/GET.test.ts +++ b/apps/server/src/api/v2/dashboards/[id]/GET.test.ts @@ -4,7 +4,7 @@ import { getCollectionsAssociatedWithDashboard, getDashboardById, getOrganizationMemberCount, - getUsersWithDashboardPermissions, + getUsersWithAssetPermissions, } from '@buster/database/queries'; import { DEFAULT_CHART_CONFIG } from '@buster/server-shared/metrics'; import { HTTPException } from 'hono/http-exception'; @@ -25,7 +25,7 @@ vi.mock('@buster/access-controls', () => ({ vi.mock('@buster/database/queries', () => ({ getDashboardById: vi.fn(), - getUsersWithDashboardPermissions: vi.fn(), + getUsersWithAssetPermissions: vi.fn(), getOrganizationMemberCount: vi.fn(), getCollectionsAssociatedWithDashboard: vi.fn(), })); @@ -49,7 +49,7 @@ vi.mock('js-yaml', () => ({ describe('getDashboardHandler', () => { const mockCheckPermission = checkPermission as Mock; const mockGetDashboardById = getDashboardById as Mock; - const mockGetUsersWithDashboardPermissions = getUsersWithDashboardPermissions as Mock; + const mockGetUsersWithAssetPermissions = getUsersWithAssetPermissions as Mock; const mockGetOrganizationMemberCount = getOrganizationMemberCount as Mock; const mockGetCollectionsAssociatedWithDashboard = getCollectionsAssociatedWithDashboard as Mock; const mockGetPubliclyEnabledByUser = getPubliclyEnabledByUser as Mock; @@ -107,7 +107,7 @@ describe('getDashboardHandler', () => { hasAccess: true, effectiveRole: 'can_view', }); - mockGetUsersWithDashboardPermissions.mockResolvedValue([]); + mockGetUsersWithAssetPermissions.mockResolvedValue([]); mockGetOrganizationMemberCount.mockResolvedValue(5); mockGetCollectionsAssociatedWithDashboard.mockResolvedValue([]); mockGetPubliclyEnabledByUser.mockResolvedValue(null); diff --git a/apps/server/src/api/v2/dashboards/[id]/GET.ts b/apps/server/src/api/v2/dashboards/[id]/GET.ts index e9d5fc97d..2cd3ab9e6 100644 --- a/apps/server/src/api/v2/dashboards/[id]/GET.ts +++ b/apps/server/src/api/v2/dashboards/[id]/GET.ts @@ -4,7 +4,7 @@ import { getCollectionsAssociatedWithDashboard, getDashboardById, getOrganizationMemberCount, - getUsersWithDashboardPermissions, + getUsersWithAssetPermissions, } from '@buster/database/queries'; import { GetDashboardParamsSchema, @@ -205,7 +205,7 @@ export async function getDashboardHandler( // Get the extra dashboard info concurrently const [individualPermissions, workspaceMemberCount, collections, publicEnabledBy] = await Promise.all([ - getUsersWithDashboardPermissions({ dashboardId }), + getUsersWithAssetPermissions({ assetId: dashboardId, assetType: 'dashboard_file' }), getOrganizationMemberCount(dashboardFile.organizationId), getCollectionsAssociatedWithDashboard(dashboardId, user.id), getPubliclyEnabledByUser(dashboardFile.publiclyEnabledBy), diff --git a/apps/server/src/shared-helpers/metric-helpers.test.ts b/apps/server/src/shared-helpers/metric-helpers.test.ts index cc440eed3..8f4dd35d3 100644 --- a/apps/server/src/shared-helpers/metric-helpers.test.ts +++ b/apps/server/src/shared-helpers/metric-helpers.test.ts @@ -5,7 +5,7 @@ import { getAssetsAssociatedWithMetric, getMetricFileById, getOrganizationMemberCount, - getUsersWithMetricPermissions, + getUsersWithAssetPermissions, } from '@buster/database/queries'; import { type ChartConfigProps, DEFAULT_CHART_CONFIG } from '@buster/server-shared/metrics'; import { HTTPException } from 'hono/http-exception'; @@ -26,7 +26,7 @@ vi.mock('@buster/access-controls', () => ({ vi.mock('@buster/database/queries', () => ({ getMetricFileById: vi.fn(), - getUsersWithMetricPermissions: vi.fn(), + getUsersWithAssetPermissions: vi.fn(), getOrganizationMemberCount: vi.fn(), getAssetsAssociatedWithMetric: vi.fn(), })); @@ -45,7 +45,7 @@ vi.mock('js-yaml', () => ({ describe('metric-helpers', () => { const mockCheckPermission = checkPermission as Mock; const mockGetMetricFileById = getMetricFileById as Mock; - const mockGetUsersWithMetricPermissions = getUsersWithMetricPermissions as Mock; + const mockGetUsersWithAssetPermissions = getUsersWithAssetPermissions as Mock; const mockGetOrganizationMemberCount = getOrganizationMemberCount as Mock; const mockGetAssetsAssociatedWithMetric = getAssetsAssociatedWithMetric as Mock; const mockGetPubliclyEnabledByUser = getPubliclyEnabledByUser as Mock; @@ -111,7 +111,7 @@ describe('metric-helpers', () => { hasAccess: true, effectiveRole: 'can_view', }); - mockGetUsersWithMetricPermissions.mockResolvedValue([]); + mockGetUsersWithAssetPermissions.mockResolvedValue([]); mockGetOrganizationMemberCount.mockResolvedValue(5); mockGetAssetsAssociatedWithMetric.mockResolvedValue({ dashboards: [], @@ -630,9 +630,9 @@ describe('metric-helpers', () => { it('should include all associated data from concurrent queries', async () => { const processedData = createProcessedData(); - mockGetUsersWithMetricPermissions.mockResolvedValue([ - { userId: 'user-1', role: 'can_view' }, - { userId: 'user-2', role: 'can_edit' }, + mockGetUsersWithAssetPermissions.mockResolvedValue([ + { role: 'can_view', email: 'user1@test.com', name: 'User 1', avatarUrl: null }, + { role: 'can_edit', email: 'user2@test.com', name: 'User 2', avatarUrl: null }, ]); mockGetOrganizationMemberCount.mockResolvedValue(10); mockGetAssetsAssociatedWithMetric.mockResolvedValue({ diff --git a/apps/server/src/shared-helpers/metric-helpers.ts b/apps/server/src/shared-helpers/metric-helpers.ts index 07a1cedb4..d85d302d6 100644 --- a/apps/server/src/shared-helpers/metric-helpers.ts +++ b/apps/server/src/shared-helpers/metric-helpers.ts @@ -5,7 +5,7 @@ import { getAssetsAssociatedWithMetric, getMetricFileById, getOrganizationMemberCount, - getUsersWithMetricPermissions, + getUsersWithAssetPermissions, } from '@buster/database/queries'; import { type ChartConfigProps, @@ -215,7 +215,7 @@ export async function buildMetricResponse( // Get the extra metric info concurrently const [individualPermissions, workspaceMemberCount, associatedAssets, publicEnabledBy] = await Promise.all([ - getUsersWithMetricPermissions({ metricId: metricFile.id }), + getUsersWithAssetPermissions({ assetId: metricFile.id, assetType: 'metric_file' }), getOrganizationMemberCount(metricFile.organizationId), getAssetsAssociatedWithMetric(metricFile.id, userId), getPubliclyEnabledByUser(metricFile.publiclyEnabledBy), diff --git a/apps/web/src/api/buster_rest/chats/requests.ts b/apps/web/src/api/buster_rest/chats/requests.ts index 0ef707585..ab47880bc 100644 --- a/apps/web/src/api/buster_rest/chats/requests.ts +++ b/apps/web/src/api/buster_rest/chats/requests.ts @@ -31,7 +31,7 @@ export const getListLogs = async (params?: GetLogsListRequest): Promise => { - return mainApi.get(`${CHATS_BASE}/${id}`).then((res) => res.data); + return mainApiV2.get(`${CHATS_BASE}/${id}`).then((res) => res.data); }; export const deleteChat = async (data: DeleteChatsRequest): Promise => { diff --git a/packages/access-controls/src/assets/cascading-permissions.ts b/packages/access-controls/src/assets/cascading-permissions.ts index 46efe9500..896473dae 100644 --- a/packages/access-controls/src/assets/cascading-permissions.ts +++ b/packages/access-controls/src/assets/cascading-permissions.ts @@ -311,6 +311,88 @@ export async function checkChatCollectionAccess(chatId: string, user: User): Pro } } +/** + * Check if a user has access to a report through any chat that contains it. + * If a user has access to a chat (direct, public, or workspace), they can view the reports in it. + */ +export async function checkReportChatAccess(reportId: string, user: User): Promise { + try { + // Get all chats containing this dashboard with their workspace sharing info + const chats = await checkChatsContainingAsset(reportId, 'report_file'); + + if (!chats || chats.length === 0) { + return false; + } + + // Check if user has access to any of these chats + for (const chat of chats) { + const hasAccess = await hasAssetPermission({ + assetId: chat.id, + assetType: 'chat' as AssetType, + userId: user.id, + requiredRole: 'can_view' as AssetPermissionRole, + organizationId: chat.organizationId, + workspaceSharing: (chat.workspaceSharing as WorkspaceSharing) ?? 'none', + publiclyAccessible: chat.publiclyAccessible, + publicExpiryDate: chat.publicExpiryDate ?? undefined, + publicPassword: undefined, // We don't support passwords on the chats table + userSuppliedPassword: undefined, // We don't support passwords on the chats table + }); + + if (hasAccess) { + return true; + } + } + + return false; + } catch (error) { + throw new AccessControlError( + 'cascading_permission_error', + 'Failed to check report chat access', + { error } + ); + } +} + +/** + * Check if a user has access to a report through any collection that contains it. + * If a user has access to a collection (direct or workspace), they can view the reports in it. + */ +export async function checkReportCollectionAccess(reportId: string, user: User): Promise { + try { + // Get all collections containing this report with their workspace sharing info + const collections = await checkCollectionsContainingAsset(reportId, 'report_file'); + + if (!collections || collections.length === 0) { + return false; + } + + // Check if user has access to any of these collections + for (const collection of collections) { + const hasAccess = await hasAssetPermission({ + assetId: collection.id, + assetType: 'collection' as AssetType, + userId: user.id, + requiredRole: 'can_view' as AssetPermissionRole, + organizationId: collection.organizationId, + workspaceSharing: (collection.workspaceSharing as WorkspaceSharing) ?? 'none', + }); + + if (hasAccess) { + return true; + } + } + + return false; + } catch (error) { + throw new AccessControlError( + 'cascading_permission_error', + 'Failed to check report collection access', + { error } + ); + } +} + /** * Check cascading permissions for an asset. * This checks if a user has access to an asset through other assets that contain it. @@ -389,12 +471,26 @@ export async function checkCascadingPermissions( break; } + case 'report_file': { + // Check access through chats and collections + const reportChatAccess = await checkReportChatAccess(assetId, user); + if (reportChatAccess) { + hasAccess = true; + break; + } + + const reportCollectionAccess = await checkReportCollectionAccess(assetId, user); + if (reportCollectionAccess) { + hasAccess = true; + break; + } + break; + } + case 'collection': - case 'report_file': - // Collections and reports don't have cascading permissions (they're top-level) hasAccess = false; break; - + // Collections don't have cascading permissions (they're top-level) default: hasAccess = false; } diff --git a/packages/access-controls/src/assets/checks.ts b/packages/access-controls/src/assets/checks.ts index 08938486f..20d91e818 100644 --- a/packages/access-controls/src/assets/checks.ts +++ b/packages/access-controls/src/assets/checks.ts @@ -113,8 +113,6 @@ export async function checkPermission(check: AssetPermissionCheck): Promise; + +export const GetUsersWithAssetPermissionsResultSchema = z.object({ + role: AssetPermissionRoleSchema, + email: z.string(), + name: z.string().nullable(), + avatarUrl: z.string().nullable(), +}); + +export type GetUsersWithAssetPermissionsResult = z.infer< + typeof GetUsersWithAssetPermissionsResultSchema +>; + +/** + * Get all users with direct permissions to any asset type + * This is a generic function that works with chats, metrics, dashboards, etc. + */ +export async function getUsersWithAssetPermissions( + input: GetUsersWithAssetPermissionsInput +): Promise { + const validated = GetUsersWithAssetPermissionsInputSchema.parse(input); + + const individualPermissions = await db + .select({ + role: assetPermissions.role, + email: users.email, + name: users.name, + avatarUrl: users.avatarUrl, + }) + .from(assetPermissions) + .innerJoin(users, eq(users.id, assetPermissions.identityId)) + .where( + and( + eq(assetPermissions.assetId, validated.assetId), + eq(assetPermissions.assetType, validated.assetType), + eq(assetPermissions.identityType, 'user'), + isNull(assetPermissions.deletedAt) + ) + ); + + return individualPermissions.map((row) => ({ + role: row.role, + email: row.email, + name: row.name, + avatarUrl: row.avatarUrl, + })); +} diff --git a/packages/database/src/queries/assets/index.ts b/packages/database/src/queries/assets/index.ts index a76be06bf..eb666e033 100644 --- a/packages/database/src/queries/assets/index.ts +++ b/packages/database/src/queries/assets/index.ts @@ -29,3 +29,10 @@ export { GetAssetLatestVersionInputSchema, type GetAssetLatestVersionInput, } from './get-asset-latest-version'; + +export { + getUsersWithAssetPermissions, + GetUsersWithAssetPermissionsInputSchema, + type GetUsersWithAssetPermissionsInput, + type GetUsersWithAssetPermissionsResult, +} from './get-users-with-asset-permissions'; diff --git a/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts b/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts index 6d3c64a59..9bc12e59f 100644 --- a/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts +++ b/packages/database/src/queries/cascading-permissions/check-chats-containing-asset.ts @@ -13,7 +13,7 @@ export interface ChatWithSharing { export async function checkChatsContainingAsset( assetId: string, - _assetType: 'metric_file' | 'dashboard_file' + _assetType: 'metric_file' | 'dashboard_file' | 'report_file' ): Promise { const result = await db .selectDistinct({ diff --git a/packages/database/src/queries/dashboards/get-users-with-dashboard-permissions-by-id.ts b/packages/database/src/queries/dashboards/get-users-with-dashboard-permissions-by-id.ts deleted file mode 100644 index ca959de44..000000000 --- a/packages/database/src/queries/dashboards/get-users-with-dashboard-permissions-by-id.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { and, eq, isNull } from 'drizzle-orm'; -import { z } from 'zod'; -import { db } from '../../connection'; -import { assetPermissions, users } from '../../schema'; -import { AssetPermissionRoleSchema, type AssetType, type IdentityType } from '../../schema-types'; - -export const GetUsersWithDashboardPermissionsInputSchema = z.object({ - dashboardId: z.string().uuid(), -}); - -export type GetUsersWithDashboardPermissionsInput = z.infer< - typeof GetUsersWithDashboardPermissionsInputSchema ->; - -export const GetUsersWithDashboardPermissionsResultSchema = z.object({ - role: AssetPermissionRoleSchema, - email: z.string(), - name: z.string().nullable(), - avatarUrl: z.string().nullable(), -}); - -export type GetUsersWithDashboardPermissionsResult = z.infer< - typeof GetUsersWithDashboardPermissionsResultSchema ->; - -/** - * Get all users with direct permissions to a dashboard - */ -export async function getUsersWithDashboardPermissions( - input: GetUsersWithDashboardPermissionsInput -): Promise { - const validated = GetUsersWithDashboardPermissionsInputSchema.parse(input); - - const individualPermissions = await db - .select({ - role: assetPermissions.role, - email: users.email, - name: users.name, - avatarUrl: users.avatarUrl, - }) - .from(assetPermissions) - .innerJoin(users, eq(users.id, assetPermissions.identityId)) - .where( - and( - eq(assetPermissions.assetId, validated.dashboardId), - eq(assetPermissions.assetType, 'dashboard_file' as AssetType), - eq(assetPermissions.identityType, 'user' as IdentityType), - isNull(assetPermissions.deletedAt) - ) - ); - - return individualPermissions.map((row) => ({ - role: row.role, - email: row.email, - name: row.name, - avatarUrl: row.avatarUrl, - })); -} diff --git a/packages/database/src/queries/dashboards/index.ts b/packages/database/src/queries/dashboards/index.ts index 35c5ba835..f25aaf7c6 100644 --- a/packages/database/src/queries/dashboards/index.ts +++ b/packages/database/src/queries/dashboards/index.ts @@ -16,13 +16,6 @@ export { type GetDashboardByIdInput, } from './get-dashboard-by-id'; -export { - getUsersWithDashboardPermissions, - GetUsersWithDashboardPermissionsInputSchema, - type GetUsersWithDashboardPermissionsInput, - type GetUsersWithDashboardPermissionsResult, -} from './get-users-with-dashboard-permissions-by-id'; - export { getCollectionsAssociatedWithDashboard, type AssociatedCollection, diff --git a/packages/database/src/queries/messages/messages.ts b/packages/database/src/queries/messages/messages.ts index 7cb94381b..48e33254e 100644 --- a/packages/database/src/queries/messages/messages.ts +++ b/packages/database/src/queries/messages/messages.ts @@ -1,7 +1,7 @@ import type { InferSelectModel } from 'drizzle-orm'; import { and, desc, eq, isNull, ne } from 'drizzle-orm'; import { db } from '../../connection'; -import { messages } from '../../schema'; +import { messages, users } from '../../schema'; export type Message = InferSelectModel; @@ -40,6 +40,20 @@ export async function getMessagesForChat(chatId: string) { .orderBy(desc(messages.createdAt)); } +/** + * Get all messages for a specific chat with user details + * @param chatId - The ID of the chat + * @returns Array of messages for the chat with user details + */ +export async function getMessagesForChatWithUserDetails(chatId: string) { + return await db + .select({ message: messages, user: users }) + .from(messages) + .innerJoin(users, eq(messages.createdBy, users.id)) + .where(and(eq(messages.chatId, chatId), isNull(messages.deletedAt))) + .orderBy(desc(messages.createdAt)); +} + /** * Get the latest message for a specific chat * @param chatId - The ID of the chat diff --git a/packages/database/src/queries/metrics/get-users-with-metric-permissions-by-id.ts b/packages/database/src/queries/metrics/get-users-with-metric-permissions-by-id.ts deleted file mode 100644 index 448f9b671..000000000 --- a/packages/database/src/queries/metrics/get-users-with-metric-permissions-by-id.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { and, eq, isNull } from 'drizzle-orm'; -import { z } from 'zod'; -import { db } from '../../connection'; -import { assetPermissions, users } from '../../schema'; -import { AssetPermissionRoleSchema, type AssetType, type IdentityType } from '../../schema-types'; - -export const GetUsersWithMetricPermissionsInputSchema = z.object({ - metricId: z.string().uuid(), -}); - -export type GetUsersWithMetricPermissionsInput = z.infer< - typeof GetUsersWithMetricPermissionsInputSchema ->; - -export const GetUsersWithMetricPermissionsResultSchema = z.object({ - role: AssetPermissionRoleSchema, - email: z.string(), - name: z.string().nullable(), - avatarUrl: z.string().nullable(), -}); - -export type GetUsersWithMetricPermissionsResult = z.infer< - typeof GetUsersWithMetricPermissionsResultSchema ->; - -/** - * Get all users with direct permissions to a metric - */ -export async function getUsersWithMetricPermissions( - input: GetUsersWithMetricPermissionsInput -): Promise { - const validated = GetUsersWithMetricPermissionsInputSchema.parse(input); - - const individualPermissions = await db - .select({ - role: assetPermissions.role, - email: users.email, - name: users.name, - avatarUrl: users.avatarUrl, - }) - .from(assetPermissions) - .innerJoin(users, eq(users.id, assetPermissions.identityId)) - .where( - and( - eq(assetPermissions.assetId, validated.metricId), - eq(assetPermissions.assetType, 'metric_file' as AssetType), - eq(assetPermissions.identityType, 'user' as IdentityType), - isNull(assetPermissions.deletedAt) - ) - ); - - return individualPermissions.map((row) => ({ - role: row.role, - email: row.email, - name: row.name, - avatarUrl: row.avatarUrl, - })); -} diff --git a/packages/database/src/queries/metrics/index.ts b/packages/database/src/queries/metrics/index.ts index 59f1069af..bb8412f94 100644 --- a/packages/database/src/queries/metrics/index.ts +++ b/packages/database/src/queries/metrics/index.ts @@ -39,11 +39,3 @@ export { type AssociatedAsset, type AssetsAssociatedWithMetric, } from './get-permissioned-asset-associations'; - -export { - getUsersWithMetricPermissions, - GetUsersWithMetricPermissionsInputSchema, - GetUsersWithMetricPermissionsResultSchema, - type GetUsersWithMetricPermissionsInput, - type GetUsersWithMetricPermissionsResult, -} from './get-users-with-metric-permissions-by-id'; diff --git a/packages/server-shared/src/chats/chat-message.types.ts b/packages/server-shared/src/chats/chat-message.types.ts index f70b6018f..31f6afc32 100644 --- a/packages/server-shared/src/chats/chat-message.types.ts +++ b/packages/server-shared/src/chats/chat-message.types.ts @@ -154,6 +154,8 @@ export const ChatMessageSchema = z.object({ post_processing_message: PostProcessingMessageSchema.optional(), }); +export type ReasoningMessage = z.infer; +export type ResponseMessage = z.infer; export type MessageRole = z.infer; export type ChatUserMessage = z.infer; export type ChatMessage = z.infer; From e146a83ff98d0b9b87cdb4b51c6291165f2c7029 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 29 Sep 2025 11:03:31 -0600 Subject: [PATCH 07/14] added-no-category-rule --- .../ai/src/agents/analyst-agent/analyst-agent-prompt.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt index 0add6d03a..9a583f89f 100644 --- a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt +++ b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt @@ -426,11 +426,13 @@ You operate in a loop to complete tasks: - `category` in chartConfig has a very specific meaning: *split into multiple parallel series*. - Most categorical-looking fields should instead be used in `colorBy`, not in `category`. - Decision Rule - - Start by asking: *Is this field defining the primary series, or just adding context?* + - Ask: *Is this field defining the primary series, or just adding context?* - Primary series split → use category - Example: *Revenue over time by region* → multiple lines (category = region). - Context only → use colorBy - Example: *Sales reps’ performance colored by region* → one bar per rep (colorBy = region). + - No secondary grouping needed → use neither + - Example: *Top 10 products by sales* → one bar per product, no colorBy. - Checklist Before Using category 1. Is the X-axis a time series and the user wants multiple lines/bars? → `category`. 2. Does the chart need parallel groups of the same measure (e.g., stacked/grouped bars)? → `category`. @@ -446,6 +448,9 @@ You operate in a loop to complete tasks: - Correct — colorBy - "Compare invdividual customers and their revenue from East vs West" → X = rep, Y = sales, `colorBy = region` → one bar per rep, East as one color/West as another color. - "Quota attainment by department" → X = rep, Y = quota %, `colorBy = department`. + - Correct — neither + - "Top 10 products by revenue" → X = product, Y = revenue, no `category` or `colorBy`. + - "Monthly revenue trend" → X = month, Y = revenue, single line, no `category` or `colorBy`. - Incorrect — misuse of category - Wrong: "Compare East vs West reps" → X = rep, Y = sales, `category = region` (creates duplicate reps, confusing grouped bars). - Wrong: "Product sales by category" when only one measure → `category = product_type` (splits into parallel series unnecessarily). From ea1ad68a5a0aa42b5bfc056c00f3855694281c22 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 29 Sep 2025 11:03:40 -0600 Subject: [PATCH 08/14] Fix: wait for deltas to finish then write --- .../communication-tools/done-tool/done-tool-execute.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/ai/src/tools/communication-tools/done-tool/done-tool-execute.ts b/packages/ai/src/tools/communication-tools/done-tool/done-tool-execute.ts index 9df74d9d6..228c20dd0 100644 --- a/packages/ai/src/tools/communication-tools/done-tool/done-tool-execute.ts +++ b/packages/ai/src/tools/communication-tools/done-tool/done-tool-execute.ts @@ -75,11 +75,13 @@ export function createDoneToolExecute(context: DoneToolContext, state: DoneToolS throw new Error('Tool call ID is required'); } - const result = await processDone(state, state.toolCallId, context.messageId, context, input); - - // Wait for all pending updates from delta/finish to complete before returning + // CRITICAL: Wait for ALL pending updates from delta/finish to complete FIRST + // This ensures execute's update is always the last one in the queue await waitForPendingUpdates(context.messageId); + // Now do the final authoritative update with the complete input + const result = await processDone(state, state.toolCallId, context.messageId, context, input); + cleanupState(state); return result; }, From 5165a0577865baf09b54ceda373f6f503b2954b7 Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Mon, 29 Sep 2025 11:07:44 -0600 Subject: [PATCH 09/14] revert unneeded export types --- packages/server-shared/src/chats/chat-message.types.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/server-shared/src/chats/chat-message.types.ts b/packages/server-shared/src/chats/chat-message.types.ts index 31f6afc32..f70b6018f 100644 --- a/packages/server-shared/src/chats/chat-message.types.ts +++ b/packages/server-shared/src/chats/chat-message.types.ts @@ -154,8 +154,6 @@ export const ChatMessageSchema = z.object({ post_processing_message: PostProcessingMessageSchema.optional(), }); -export type ReasoningMessage = z.infer; -export type ResponseMessage = z.infer; export type MessageRole = z.infer; export type ChatUserMessage = z.infer; export type ChatMessage = z.infer; From 7cd8f23ba7eadabf7f6e323b659f6e962a263dad Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 29 Sep 2025 11:08:58 -0600 Subject: [PATCH 10/14] revert metric-tool-description.txt to staging version --- .../helpers/metric-tool-description.txt | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt b/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt index 56f91f9f8..12ab80021 100644 --- a/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt +++ b/packages/ai/src/tools/visualization-tools/metrics/helpers/metric-tool-description.txt @@ -547,12 +547,30 @@ definitions: type: array items: type: string - description: LOWERCASE column name from SQL for category grouping. + description: | + LOWERCASE column name from SQL for category grouping. + + Category vs ColorBy: + - Use CATEGORY: When you want to create multiple series/lines (one per category value). Creates separate data points and enables legend. + - Use COLORBY: When you want to apply colors to bars/points based on a column value, but keep them as a single series. + + Example: Sales by month + - With category=['region']: Creates separate lines/bars for each region (North, South, East, West) + - With colorBy=['region']: Colors bars by region but keeps them as one series tooltip: type: [array, 'null'] items: type: string description: Column names for tooltip. If null, y axis is used. Default: null + colorBy: + type: [array, 'null'] + items: + type: string + description: | + Optional array of column names to apply colors based on column values. + Use this when you want visual differentiation without creating separate series. + Perfect for: Status indicators (red/yellow/green), priority levels, or any categorical color coding. + Example: ['region'] - colors bars by region values required: - x - y @@ -608,6 +626,9 @@ definitions: type: array items: type: string + description: | + LOWERCASE column name from SQL for category grouping. + Creates separate colored series of points for each category value. size: type: array items: @@ -618,6 +639,14 @@ definitions: items: type: string description: Column names for tooltip. If null, y axis is used. Default: null + colorBy: + type: [array, 'null'] + items: + type: string + description: | + Optional array of column names to apply colors to scatter points based on column values. + Use when you want all points in one series with color-coded differentiation. + Example: ['priority'] - colors points by priority values required: - x - y @@ -649,6 +678,14 @@ definitions: items: type: string description: Column names for tooltip. If null, y axis is used. Default: null + colorBy: + type: [array, 'null'] + items: + type: string + description: | + Optional array of column names for pie slice colors based on column values. + Allows custom color mapping for specific slice categories. + Example: ['category'] - colors slices by category values required: - x - y @@ -683,12 +720,22 @@ definitions: type: array items: type: string - description: LOWERCASE column name from SQL for category grouping. + description: | + LOWERCASE column name from SQL for category grouping. + Creates separate series for each category value in combo chart. tooltip: type: [array, 'null'] items: type: string description: Column names for tooltip. If null, y axis is used. Default: null + colorBy: + type: [array, 'null'] + items: + type: string + description: | + Optional array of column names to apply colors based on column values in combo chart. + Useful for color-coding bars while lines remain as separate series. + Example: ['region'] - colors bars by region values required: - x - y From d60bc12d6d10ca13a4a6d156f51a4bd5144ee84b Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Mon, 29 Sep 2025 11:12:44 -0600 Subject: [PATCH 11/14] remove logging statement --- apps/server/src/api/v2/chats/[id]/GET.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/server/src/api/v2/chats/[id]/GET.ts b/apps/server/src/api/v2/chats/[id]/GET.ts index d95547741..8925bec71 100644 --- a/apps/server/src/api/v2/chats/[id]/GET.ts +++ b/apps/server/src/api/v2/chats/[id]/GET.ts @@ -53,7 +53,6 @@ export async function getChatHandler(params: GetChatHandlerParams): Promise Date: Mon, 29 Sep 2025 11:25:39 -0600 Subject: [PATCH 12/14] Update packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --- packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt index 9a583f89f..d50717e19 100644 --- a/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt +++ b/packages/ai/src/agents/analyst-agent/analyst-agent-prompt.txt @@ -446,7 +446,7 @@ You operate in a loop to complete tasks: - "Show monthly revenue by region" → X = month, Y = revenue, `category = region` → multiple lines. - "Stacked bars of sales by product category" → X = category, Y = sales, `category = product_type`. - Correct — colorBy - - "Compare invdividual customers and their revenue from East vs West" → X = rep, Y = sales, `colorBy = region` → one bar per rep, East as one color/West as another color. + - "Compare individual customers and their revenue from East vs West" → X = rep, Y = sales, `colorBy = region` → one bar per rep, East as one color/West as another color. - "Quota attainment by department" → X = rep, Y = quota %, `colorBy = department`. - Correct — neither - "Top 10 products by revenue" → X = product, Y = revenue, no `category` or `colorBy`. From df3b07e4d4c0dc5e8c93c6a31def7643e9b89785 Mon Sep 17 00:00:00 2001 From: Blake Rouse Date: Mon, 29 Sep 2025 11:25:22 -0600 Subject: [PATCH 13/14] added-to-think-and-prep --- ...nk-and-prep-agent-investigation-prompt.txt | 39 ++++++++----------- .../think-and-prep-agent-standard-prompt.txt | 39 ++++++++----------- 2 files changed, 34 insertions(+), 44 deletions(-) diff --git a/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-investigation-prompt.txt b/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-investigation-prompt.txt index e069342b7..64ca07462 100644 --- a/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-investigation-prompt.txt +++ b/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-investigation-prompt.txt @@ -765,28 +765,23 @@ If all true → proceed to submit prep for Asset Creation with `submitThoughts`. - if you are building a table of customers, the first column should be their name. - If you are building a table comparing regions, have the first column be region. - If you are building a column comparing regions but each row is a customer, have the first column be customer name and the second be the region but have it ordered by region so customers of the same region are next to each other. -- **Using "category" vs. "colorBy" Guidelines** - - When adding dimensions to a bar or line chart, carefully decide whether to use `category` or `colorBy`. - - **Use `category` when you want to create separate series or groups of values.** - - Each category value generates its own line, bar group, or stacked section. - - Examples: - - Line chart with revenue by month split by region → `category: [region]` - - Grouped bar chart of sales by product split by channel → `category: [channel]` - - Rule of thumb: use `category` when the visualization should **separate and compare multiple data series**. - - **Use `colorBy` when you want to keep a single series (no grouping, stacking, etc) but visually differentiate elements by color.** - - Bars or lines remain part of one series, but the colors vary by the field. - - Examples: - - Bar chart of sales by sales rep, with bars colored by region → `colorBy: [region]` - - Line chart of monthly revenue, with points/segments colored by product line → `colorBy: [product_line]` - - Rule of thumb: use `colorBy` when the visualization should **highlight categories inside a single series** rather than split into multiple groups. - - **Guidance by chart type**: - - Bar/Line: - - `category` → multiple series (grouped/stacked bars, multiple lines). - - `colorBy` → single series, colored by attribute. - - **Quick heuristic**: - - Ask: “Does the user want multiple grouped/stacked series, or just one series with colored differentiation?” - - Multiple → `category` - - One series, just colored → `colorBy` +- Using a category as "series grouping" vs. "color grouping" (categories/grouping rules for bar and line charts) + - Many attributes are categorical (labels, enums), but this does **not** mean they should create multiple series. + - Series grouping has a very specific meaning: *split into multiple parallel series that align across the X-axis*. + - Color grouping assigns colors within a single series and **does not** create parallel series. + - Misusing series grouping to “separate colors” causes empty slots or duplicated labels when categories don’t exist for every item/time — resulting in a janky chart with gaps. + - Decision Rule + - Ask: *Is this category defining the primary comparison structure, or just distinguishing items?* + - Primary structure split → use series grouping + - Example: *Values over time by group* → multiple lines (one per group). + - Distinguishing only → use color grouping + - Example: *Items on one axis, colored by group* → one bar/line per item, colored by group. + - No secondary distinction needed → use neither + - Example: *Top N items by value* → one bar per item, no color grouping. + - Checklist Before Using series grouping + 1. Is the X-axis temporal and the intent is to compare multiple parallel trends? → series grouping. + 2. Do you need grouped/stacked comparisons of the **same** measure across multiple categories? → series grouping. + 3. Otherwise (entity list on X with a single measure on Y) → keep a single series; no category/color grouping needed. - Planning and Description Guidelines - For grouped/stacked bar charts, specify the grouping/stacking field (e.g., "grouped by `[field_name]`"). - For bar charts with time units (e.g., days of the week, months, quarters, years) on the x-axis, sort the bars in chronological order rather than in ascending or descending order based on the y-axis measure. diff --git a/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-standard-prompt.txt b/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-standard-prompt.txt index b58c1ad18..6c78af87f 100644 --- a/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-standard-prompt.txt +++ b/packages/ai/src/agents/think-and-prep-agent/think-and-prep-agent-standard-prompt.txt @@ -619,28 +619,23 @@ When in doubt, be more thorough rather than less. Reports are the default becaus - if you are building a table of customers, the first column should be their name. - If you are building a table comparing regions, have the first column be region. - If you are building a column comparing regions but each row is a customer, have the first column be customer name and the second be the region but have it ordered by region so customers of the same region are next to each other. -- **Using "category" vs. "colorBy" Guidelines** - - When adding dimensions to a bar or line chart, carefully decide whether to use `category` or `colorBy`. - - **Use `category` when you want to create separate series or groups of values.** - - Each category value generates its own line, bar group, or stacked section. - - Examples: - - Line chart with revenue by month split by region → `category: [region]` - - Grouped bar chart of sales by product split by channel → `category: [channel]` - - Rule of thumb: use `category` when the visualization should **separate and compare multiple data series**. - - **Use `colorBy` when you want to keep a single series (no grouping, stacking, etc) but visually differentiate elements by color.** - - Bars or lines remain part of one series, but the colors vary by the field. - - Examples: - - Bar chart of sales by sales rep, with bars colored by region → `colorBy: [region]` - - Line chart of monthly revenue, with points/segments colored by product line → `colorBy: [product_line]` - - Rule of thumb: use `colorBy` when the visualization should **highlight categories inside a single series** rather than split into multiple groups. - - **Guidance by chart type**: - - Bar/Line: - - `category` → multiple series (grouped/stacked bars, multiple lines). - - `colorBy` → single series, colored by attribute. - - **Quick heuristic**: - - Ask: “Does the user want multiple grouped/stacked series, or just one series with colored differentiation?” - - Multiple → `category` - - One series, just colored → `colorBy` +- Using a category as "series grouping" vs. "color grouping" (categories/grouping rules for bar and line charts) + - Many attributes are categorical (labels, enums), but this does **not** mean they should create multiple series. + - Series grouping has a very specific meaning: *split into multiple parallel series that align across the X-axis*. + - Color grouping assigns colors within a single series and **does not** create parallel series. + - Misusing series grouping to “separate colors” causes empty slots or duplicated labels when categories don’t exist for every item/time — resulting in a janky chart with gaps. + - Decision Rule + - Ask: *Is this category defining the primary comparison structure, or just distinguishing items?* + - Primary structure split → use series grouping + - Example: *Values over time by group* → multiple lines (one per group). + - Distinguishing only → use color grouping + - Example: *Items on one axis, colored by group* → one bar/line per item, colored by group. + - No secondary distinction needed → use neither + - Example: *Top N items by value* → one bar per item, no color grouping. + - Checklist Before Using series grouping + 1. Is the X-axis temporal and the intent is to compare multiple parallel trends? → series grouping. + 2. Do you need grouped/stacked comparisons of the **same** measure across multiple categories? → series grouping. + 3. Otherwise (entity list on X with a single measure on Y) → keep a single series; no category/color grouping needed. - Planning and Description Guidelines - For grouped/stacked bar charts, specify the grouping/stacking field (e.g., "grouped by `[field_name]`"). - For bar charts with time units (e.g., days of the week, months, quarters, years) on the x-axis, sort the bars in chronological order rather than in ascending or descending order based on the y-axis measure. From a72f00a70faa7b345056937a3515d179c7c84068 Mon Sep 17 00:00:00 2001 From: Wells Bunker Date: Mon, 29 Sep 2025 11:32:57 -0600 Subject: [PATCH 14/14] test fix --- apps/server/src/api/v2/chats/services/chat-service.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apps/server/src/api/v2/chats/services/chat-service.test.ts b/apps/server/src/api/v2/chats/services/chat-service.test.ts index 51d8351d1..68d8385ff 100644 --- a/apps/server/src/api/v2/chats/services/chat-service.test.ts +++ b/apps/server/src/api/v2/chats/services/chat-service.test.ts @@ -3,12 +3,12 @@ import { ChatError, ChatErrorCode } from '@buster/server-shared/chats'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { initializeChat } from './chat-service'; -import { canUserAccessChatCached, checkPermission } from '@buster/access-controls'; +import { checkPermission } from '@buster/access-controls'; // Import mocked functions import { + createChat, createMessage, getChatWithDetails, - getMessagesForChat, getMessagesForChatWithUserDetails, getOrganizationMemberCount, getUsersWithAssetPermissions, @@ -134,13 +134,11 @@ describe('chat-service', () => { }); it('should add message to existing chat when chat_id is provided', async () => { - vi.mocked(canUserAccessChatCached).mockResolvedValue(true); vi.mocked(getChatWithDetails).mockResolvedValue({ chat: mockChat, user: { id: 'user-123', name: 'Test User', avatarUrl: null } as any, isFavorited: false, }); - vi.mocked(getMessagesForChat).mockResolvedValue([mockMessage]); vi.mocked(createMessage).mockResolvedValue({ ...mockMessage, id: 'msg-456',