Conversation
There was a problem hiding this comment.
Pull request overview
This PR reworks the AI dashboard generation feature to use an autonomous tool-based approach where the AI discovers database tables independently, rather than requiring a specific table name as input.
Changes:
- Replaced two-step AI workflow (suggestions + panel generation) with single tool-loop workflow using
getTablesListandgetTableStructuretools - Removed
tableNamequery parameter requirement from the API endpoint (breaking change) - Changed transaction mode from ON to OFF for dashboard generation
- Updated test mocks to simulate tool-based AI interaction pattern
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
backend/test/ava-tests/saas-tests/dashboard-ai-generate-table-dashboard-e2e.test.ts |
Updated test mocks to simulate new tool-based AI workflow; removed tests for tableName validation; contains debug console.log statements |
backend/test/ava-tests/non-saas-tests/non-saas-dashboard-ai-generate-table-dashboard-e2e.test.ts |
Mirror of saas test updates; updated mocks and removed tableName validation tests; contains debug console.log statements |
backend/src/entities/visualizations/panel-position/use-cases/generate-table-dashboard-with-ai.use.case.ts |
Core logic rework: implemented tool loop for autonomous table discovery, updated prompts, modified panel validation flow, removed table-specific context requirements |
backend/src/entities/visualizations/panel-position/panel-position.controller.ts |
Removed tableName query parameter from endpoint; updated API documentation; changed transaction mode from ON to OFF |
backend/src/entities/visualizations/panel-position/data-structures/generate-table-dashboard-with-ai.ds.ts |
Removed table_name field from data structure |
backend/src/ai-core/tools/database-tools.ts |
Added createDashboardGenerationTools() function with two new tools: getTablesList and getTableStructure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async generateTableDashboardWithAi( | ||
| @SlugUuid('connectionId') connectionId: string, |
There was a problem hiding this comment.
This is a breaking API change. The endpoint previously required a tableName query parameter and now it doesn't. Any existing API clients that call this endpoint with the tableName parameter will need to be updated. Consider documenting this breaking change in release notes or providing backward compatibility if needed.
| @@ -282,57 +328,3 @@ test.serial(`${currentTest} should reject when all AI panels have unsafe queries | |||
| const errorResponse = JSON.parse(generateDashboard.text); | |||
| t.truthy(errorResponse.message); | |||
| }); | |||
There was a problem hiding this comment.
Two test cases have been removed that validated important error scenarios: 1) handling of non-existent tables, and 2) validation that tableName was required. While the tableName parameter is no longer required (which is intentional), the test for non-existent tables was still valuable to verify error handling. Consider adding a test to verify that the AI properly handles cases where the database has no tables or invalid table structures.
| }); | |
| }); | |
| test.serial(`${currentTest} should handle databases with no tables or invalid structures`, async (t) => { | |
| // Use a non-existent database name to simulate a connection whose schema cannot be inspected. | |
| mockDashboardResponse = MOCK_DASHBOARD_RESPONSE; | |
| toolCallCounter = 0; | |
| const baseConnection = getTestData(mockFactory).connectionToPostgres; | |
| const connectionToEmptyDB = { | |
| ...baseConnection, | |
| database: `${baseConnection.database}_non_existent_${faker.string.alphanumeric(8)}` | |
| }; | |
| const { token } = await registerUserAndReturnUserInfo(app); | |
| const createConnectionResponse = await request(app.getHttpServer()) | |
| .post('/connection') | |
| .send(connectionToEmptyDB) | |
| .set('Cookie', token) | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| t.is(createConnectionResponse.status, 201); | |
| const connectionId = JSON.parse(createConnectionResponse.text).id; | |
| const generateDashboard = await request(app.getHttpServer()) | |
| .post(`/dashboard/generate-table-dashboard/${connectionId}`) | |
| .send({}) | |
| .set('Cookie', token) | |
| .set('masterpwd', 'ahalaimahalai') | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| // Depending on implementation, this may be a 4xx validation error or a 5xx internal error. | |
| t.true([400, 500].includes(generateDashboard.status)); | |
| const errorResponse = JSON.parse(generateDashboard.text); | |
| t.truthy(errorResponse.message || errorResponse.error || errorResponse.details); | |
| }); |
| @@ -288,57 +327,3 @@ test.serial(`${currentTest} should reject when all AI panels have unsafe queries | |||
| const errorResponse = JSON.parse(generateDashboard.text); | |||
| t.truthy(errorResponse.message); | |||
| }); | |||
There was a problem hiding this comment.
Two test cases have been removed that validated important error scenarios: 1) handling of non-existent tables, and 2) validation that tableName was required. While the tableName parameter is no longer required (which is intentional), the test for non-existent tables was still valuable to verify error handling. Consider adding a test to verify that the AI properly handles cases where the database has no tables or invalid table structures.
| }); | |
| }); | |
| test.serial(`${currentTest} should handle generate-table-dashboard when database has no tables`, async (t) => { | |
| // Use the normal AI dashboard response; error should come from schema inspection, not AI output | |
| mockDashboardResponse = MOCK_DASHBOARD_RESPONSE; | |
| toolCallCounter = 0; | |
| const connectionToTestDB = getTestData(mockFactory).connectionToPostgres; | |
| const { token } = await registerUserAndReturnUserInfo(app); | |
| // Intentionally do NOT create any test tables for this connection, to simulate an empty database schema | |
| const createConnectionResponse = await request(app.getHttpServer()) | |
| .post('/connection') | |
| .send(connectionToTestDB) | |
| .set('Cookie', token) | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| const connectionId = JSON.parse(createConnectionResponse.text).id; | |
| t.is(createConnectionResponse.status, 201); | |
| const generateDashboard = await request(app.getHttpServer()) | |
| .post(`/dashboard/generate-table-dashboard/${connectionId}`) | |
| .send({}) | |
| .set('Cookie', token) | |
| .set('masterpwd', 'ahalaimahalai') | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| // Expect the service to fail gracefully when there are no tables to build a dashboard from | |
| t.is(generateDashboard.status, 400); | |
| const errorResponse = JSON.parse(generateDashboard.text); | |
| t.truthy(errorResponse.message); | |
| }); |
| const MOCK_TABLES_LIST = [ | ||
| { tableName: 'test_table', isView: false }, | ||
| ]; | ||
|
|
||
| let mockDashboardSuggestion = MOCK_DASHBOARD_SUGGESTION; | ||
| let mockPanelResponse = MOCK_PANEL_RESPONSE; | ||
| const MOCK_TABLE_STRUCTURE = [ | ||
| { column_name: 'id', data_type: 'integer', allow_null: false }, | ||
| { column_name: 'name', data_type: 'varchar', allow_null: true }, | ||
| ]; |
There was a problem hiding this comment.
MOCK_TABLES_LIST and MOCK_TABLE_STRUCTURE are defined but never used in the tests. Consider removing these unused constants or adding assertions that verify the tool execution with these mock values if they were intended to be used.
|
|
||
| if (tableInfo.columns.length === 0) { | ||
| throw new BadRequestException(`The specified table "${table_name}" does not have any columns or does not exist.`); | ||
| let userEmail: string; |
There was a problem hiding this comment.
The userEmail variable is only initialized for agent connection types (line 110-112), but is used unconditionally in the tool execution (line 335, 345). For non-agent connections, userEmail will be undefined. While the DAO methods might handle undefined/null userEmail, this should be made explicit or documented to avoid potential runtime issues.
| let userEmail: string; | |
| let userEmail: string | null = null; |
| .set('Accept', 'application/json'); | ||
|
|
||
| const generateDashboardRO = JSON.parse(generateDashboard.text); | ||
| console.log('🚀 ~ generateDashboardRO:', generateDashboardRO) |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging to production.
| console.log('🚀 ~ generateDashboardRO:', generateDashboardRO) |
| .set('masterpwd', 'ahalaimahalai') | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
| console.log('🚀 ~ generateDashboard:', generateDashboard.text) |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging to production.
| console.log('🚀 ~ generateDashboard:', generateDashboard.text) |
| dashboard_name: generateDto.dashboard_name, | ||
| }; | ||
| return await this.generateTableDashboardWithAiUseCase.execute(inputData, InTransactionEnum.ON); | ||
| return await this.generateTableDashboardWithAiUseCase.execute(inputData, InTransactionEnum.OFF); |
There was a problem hiding this comment.
The transaction mode has been changed from ON to OFF. This means that if an error occurs during dashboard generation after the dashboard entity is saved, the database could be left in an inconsistent state with a partially created dashboard. Consider whether this change is intentional and if proper cleanup logic exists for error cases.
| return await this.generateTableDashboardWithAiUseCase.execute(inputData, InTransactionEnum.OFF); | |
| return await this.generateTableDashboardWithAiUseCase.execute(inputData, InTransactionEnum.ON); |
No description provided.