feat: implement schema change chat functionality with message persistence and thread management#1770
Merged
Merged
Conversation
…ence and thread management
📝 WalkthroughWalkthroughThis PR adds conversational threading to schema-change generation, enabling users to refine schema proposals across multiple turns. It introduces chat entities, repositories, and persistence logic that maintains message history, automatically generates chat names, and validates thread ownership across database connections. ChangesSchema Change Chat Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements schema-change chat threads for the table-schema generation flow, persisting user/AI turns and allowing follow-up prompts to reuse prior conversation context.
Changes:
- Adds schema-change chat/message entities, repositories, database context wiring, and migration.
- Extends generate schema-change request/response DTOs and controller handling with optional
threadId. - Updates generation use case to create/resolve threads, persist messages, update last batch, and adds e2e coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
backend/test/ava-tests/non-saas-tests/non-saas-table-schema-postgres-e2e.test.ts |
Adds e2e tests for thread creation, continuation, and cross-connection rejection. |
backend/src/migrations/1778767036234-AddSchemaChangeChatEntities.ts |
Creates chat/message tables and related constraints. |
backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts |
Adds thread resolution, history construction, message persistence, and chat naming. |
backend/src/entities/table-schema/table-schema.module.ts |
Registers new chat/message entities with TypeORM module features. |
backend/src/entities/table-schema/table-schema.controller.ts |
Passes optional body threadId into generation use case. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat/schema-change-chat.entity.ts |
Defines schema-change chat thread entity. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat/repository/schema-change-chat-repository.interface.ts |
Defines chat repository contract. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat/repository/schema-change-chat-repository.extension.ts |
Implements chat repository methods. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/schema-change-chat-message.entity.ts |
Defines persisted chat message entity. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.interface.ts |
Defines chat message repository contract. |
backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.extension.ts |
Implements chat message persistence/query helpers. |
backend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.ts |
Adds threadId to generate response DTO. |
backend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.ts |
Adds optional request body threadId validation/docs. |
backend/src/entities/table-schema/application/data-structures/generate-schema-change.ds.ts |
Adds threadId to internal use-case input. |
backend/src/common/application/global-database-context.ts |
Wires new repositories into global database context. |
backend/src/common/application/global-database-context.interface.ts |
Exposes new repositories on the database context interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await queryRunner.query( | ||
| `CREATE TABLE "schema_change_chat_message" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "message" text, "role" "public"."schema_change_chat_message_role_enum", "batch_id" uuid, "created_at" TIMESTAMP NOT NULL DEFAULT now(), "updated_at" TIMESTAMP DEFAULT now(), "chat_id" uuid NOT NULL, CONSTRAINT "PK_5984cdb248fa9c2f55f5a19022c" PRIMARY KEY ("id"))`, | ||
| ); | ||
| await queryRunner.query(`ALTER TABLE "ai_chat_message" DROP COLUMN "response_id"`); |
Comment on lines
+180
to
+185
| if (existing) { | ||
| if (existing.connection_id !== connectionId) { | ||
| throw new BadRequestException('Provided threadId belongs to a different connection.'); | ||
| } | ||
| return { chat: existing, isNewChat: false }; | ||
| } |
| ? createElasticsearchSchemaChangeTools() | ||
| : createSchemaChangeTools(); | ||
|
|
||
| await this._dbContext.schemaChangeChatMessageRepository.saveMessage(chat.id, userPrompt, MessageRole.user); |
Comment on lines
+201
to
+213
| const previousMessages = await this._dbContext.schemaChangeChatMessageRepository.findMessagesForChat(chatId); | ||
| const builder = new MessageBuilder().system(systemPrompt); | ||
|
|
||
| for (const msg of previousMessages) { | ||
| if (msg.role === MessageRole.user) { | ||
| builder.human(msg.message); | ||
| } else if (msg.role === MessageRole.ai) { | ||
| builder.ai(msg.message); | ||
| } | ||
| } | ||
|
|
||
| builder.human(userMessage); | ||
| return builder.build(); |
| `CREATE TYPE "public"."schema_change_chat_message_role_enum" AS ENUM('user', 'ai', 'system')`, | ||
| ); | ||
| await queryRunner.query( | ||
| `CREATE TABLE "schema_change_chat_message" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "message" text, "role" "public"."schema_change_chat_message_role_enum", "batch_id" uuid, "created_at" TIMESTAMP NOT NULL DEFAULT now(), "updated_at" TIMESTAMP DEFAULT now(), "chat_id" uuid NOT NULL, CONSTRAINT "PK_5984cdb248fa9c2f55f5a19022c" PRIMARY KEY ("id"))`, |
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `CREATE TABLE "schema_change_chat" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "name" character varying, "created_at" TIMESTAMP NOT NULL DEFAULT now(), "updated_at" TIMESTAMP DEFAULT now(), "user_id" uuid NOT NULL, "connection_id" character varying(38) NOT NULL, "last_batch_id" uuid, CONSTRAINT "PK_60082e3e240c265fc043290381d" PRIMARY KEY ("id"))`, |
| required: false, | ||
| nullable: true, | ||
| description: | ||
| 'Conversation thread ID. Present when the request used or created a chat thread. Pass it back as the threadId query param on subsequent generate calls to continue the conversation with full prior context.', |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
backend/src/migrations/1778767036234-AddSchemaChangeChatEntities.ts (1)
6-26: ⚡ Quick winConsider adding indexes on the new foreign-key columns.
PostgreSQL does not automatically index FK columns. The cascade-delete paths (
user→chats,connection→chats,schema_change_chat→messages) and the typical query patterns ("list chats for user/connection", "list messages for chat") will benefit from indexes onschema_change_chat(user_id),schema_change_chat(connection_id), andschema_change_chat_message(chat_id). Without them, parent deletes and history loads will scan these tables.♻️ Suggested indexes (add to
up()and reverse indown())await queryRunner.query( `ALTER TABLE "schema_change_chat_message" ADD CONSTRAINT "FK_32825f4780664738f60fa75cd50" FOREIGN KEY ("chat_id") REFERENCES "schema_change_chat"("id") ON DELETE CASCADE ON UPDATE NO ACTION`, ); + await queryRunner.query(`CREATE INDEX "IDX_schema_change_chat_user_id" ON "schema_change_chat" ("user_id")`); + await queryRunner.query(`CREATE INDEX "IDX_schema_change_chat_connection_id" ON "schema_change_chat" ("connection_id")`); + await queryRunner.query(`CREATE INDEX "IDX_schema_change_chat_message_chat_id" ON "schema_change_chat_message" ("chat_id")`); }public async down(queryRunner: QueryRunner): Promise<void> { + await queryRunner.query(`DROP INDEX "IDX_schema_change_chat_message_chat_id"`); + await queryRunner.query(`DROP INDEX "IDX_schema_change_chat_connection_id"`); + await queryRunner.query(`DROP INDEX "IDX_schema_change_chat_user_id"`); await queryRunner.query( `ALTER TABLE "schema_change_chat_message" DROP CONSTRAINT "FK_32825f4780664738f60fa75cd50"`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/migrations/1778767036234-AddSchemaChangeChatEntities.ts` around lines 6 - 26, The migration creates schema_change_chat and schema_change_chat_message and adds FKs but does not create indexes on the FK columns; add CREATE INDEX statements for schema_change_chat(user_id), schema_change_chat(connection_id) and schema_change_chat_message(chat_id) inside the up() method (after the corresponding CREATE TABLE/ALTER TABLE statements) and add corresponding DROP INDEX statements in the down() method to reverse the change so deletes and common queries are not forced to do full-table scans; reference the up() method and the tables schema_change_chat and schema_change_chat_message and the columns user_id, connection_id, chat_id when making the changes.backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.extension.ts (1)
13-19: ⚡ Quick winConsider using entity class instead of string for type safety.
Line 16 uses the string
'schema_change_chat_message'as the table reference, while the entity classSchemaChangeChatMessageEntityis available. Using the entity class provides better type safety and refactoring support.♻️ Proposed refactor
async deleteMessagesForChat(chatId: string): Promise<void> { await this.createQueryBuilder() .delete() - .from('schema_change_chat_message') + .from(SchemaChangeChatMessageEntity) .where('chat_id = :chatId', { chatId }) .execute(); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.extension.ts` around lines 13 - 19, The deleteMessagesForChat method uses the literal table name 'schema_change_chat_message' which is fragile; change it to use the entity class SchemaChangeChatMessageEntity to gain type safety and refactor resilience. Update the query in deleteMessagesForChat to reference the entity (e.g., pass SchemaChangeChatMessageEntity to createQueryBuilder or to .from) instead of the string, keep the same where parameter (chatId) and .execute() call, and ensure any import of SchemaChangeChatMessageEntity is added at the top of the file.backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts (1)
103-106: 💤 Low valueRedundant error handler in fire-and-forget call.
The
.catch()handler at line 103-105 is redundant becausegenerateAndUpdateChatNamealready has an internal try-catch that captures exceptions to Sentry (line 248-250) without re-throwing. The outer.catch()will never execute.♻️ Simplification
if (isNewChat) { - this.generateAndUpdateChatName(chat.id, userPrompt).catch((error) => { - Sentry.captureException(error); - }); + this.generateAndUpdateChatName(chat.id, userPrompt); }The internal error handling in
generateAndUpdateChatName(lines 226-250) already captures exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts` around lines 103 - 106, The call site uses a redundant .catch on the fire-and-forget call to generateAndUpdateChatName(chat.id, userPrompt) even though generateAndUpdateChatName already handles and Sentry-captures its own errors; remove the trailing .catch((error) => { Sentry.captureException(error); }) so the call becomes a plain fire-and-forget invocation, or if you intended outer handling instead, change generateAndUpdateChatName to rethrow errors and use await + .catch here—refer to generateAndUpdateChatName for the internal try/catch and the caller where generateAndUpdateChatName(chat.id, userPrompt).catch(...) is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.ts`:
- Around line 19-29: The DTO's `@ApiProperty` marks threadId as nullable but the
TypeScript declaration excludes null; update the property in
generate-schema-change.dto.ts so the TypeScript type allows null (e.g.,
threadId?: string | null) and keep the existing decorators (`@ApiProperty` with
nullable: true, `@IsOptional`(), `@IsUUID`()) so validation still skips
null/undefined and UUID validation runs for non-null values.
In
`@backend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.ts`:
- Around line 21-22: The response DTO's description for the threadId field
incorrectly tells clients to pass it as a query param; update the docstring for
the threadId property in SchemaChangeBatchResponseDto (the DTO where threadId is
declared) to state that threadId should be returned by the server and included
in subsequent requests in the request body (not as a query param), e.g.,
"Present when the request used or created a chat thread. Include this threadId
in the request body of subsequent generate calls to continue the conversation
with full prior context."
In
`@backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/schema-change-chat-message.entity.ts`:
- Around line 22-23: The entity column for role currently allows null even
though saveMessage(...) requires a role; change the Column on
schema-change-chat-message.entity (the role property of type MessageRole) to be
non-nullable (nullable: false) and remove the default null so the DB enforces
presence; additionally add a migration to backfill or reject existing null rows
(e.g., set a sensible default role or fail migration) and apply the schema
change so the database constraint matches the saveMessage(...) contract.
In
`@backend/src/entities/table-schema/schema-change-chat/schema-change-chat/schema-change-chat.entity.ts`:
- Around line 21-22: The entity property "name" is declared as type string but
the Column decorator lacks nullable: true while createChatForUser writes name:
null; update the SchemaChangeChat entity by adding nullable: true to the `@Column`
on the name property (and/or adjust the TypeScript type to string | null) so the
DB mapping and the write path (createChatForUser) are consistent; ensure you
modify the Column decorator on the name field and the property type accordingly.
In
`@backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts`:
- Around line 102-106: The call to generateAndUpdateChatName in the isNewChat
branch is fire-and-forget and can fail because GlobalDatabaseContext is
request-scoped and may be destroyed before the async work completes; either
await the call (await this.generateAndUpdateChatName(chat.id, userPrompt)) so
the request lifecycle keeps the DB context alive and handle errors with
try/catch + Sentry.captureException, or instead delegate the task to a
singleton/background worker (e.g., a ChatNameGenerationService.enqueue(chat.id,
userPrompt) or BackgroundJobService) that performs the DB write outside the
request scope; update the code that currently calls
this.generateAndUpdateChatName(...).catch(...) accordingly.
- Around line 173-189: The resolveChat method currently creates a new chat when
a threadId is supplied but not found/owned by the user; change it so that if a
non-null threadId is provided and findChatByIdAndUserId returns no chat, you
throw a BadRequestException instead of falling through to createChatForUser.
Keep the existing connection_id check (existing.connection_id !== connectionId)
and only call createChatForUser when threadId is null; update the logic in
resolveChat to detect the "threadId supplied but missing" case and raise an
error.
---
Nitpick comments:
In
`@backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.extension.ts`:
- Around line 13-19: The deleteMessagesForChat method uses the literal table
name 'schema_change_chat_message' which is fragile; change it to use the entity
class SchemaChangeChatMessageEntity to gain type safety and refactor resilience.
Update the query in deleteMessagesForChat to reference the entity (e.g., pass
SchemaChangeChatMessageEntity to createQueryBuilder or to .from) instead of the
string, keep the same where parameter (chatId) and .execute() call, and ensure
any import of SchemaChangeChatMessageEntity is added at the top of the file.
In
`@backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts`:
- Around line 103-106: The call site uses a redundant .catch on the
fire-and-forget call to generateAndUpdateChatName(chat.id, userPrompt) even
though generateAndUpdateChatName already handles and Sentry-captures its own
errors; remove the trailing .catch((error) => { Sentry.captureException(error);
}) so the call becomes a plain fire-and-forget invocation, or if you intended
outer handling instead, change generateAndUpdateChatName to rethrow errors and
use await + .catch here—refer to generateAndUpdateChatName for the internal
try/catch and the caller where generateAndUpdateChatName(chat.id,
userPrompt).catch(...) is used.
In `@backend/src/migrations/1778767036234-AddSchemaChangeChatEntities.ts`:
- Around line 6-26: The migration creates schema_change_chat and
schema_change_chat_message and adds FKs but does not create indexes on the FK
columns; add CREATE INDEX statements for schema_change_chat(user_id),
schema_change_chat(connection_id) and schema_change_chat_message(chat_id) inside
the up() method (after the corresponding CREATE TABLE/ALTER TABLE statements)
and add corresponding DROP INDEX statements in the down() method to reverse the
change so deletes and common queries are not forced to do full-table scans;
reference the up() method and the tables schema_change_chat and
schema_change_chat_message and the columns user_id, connection_id, chat_id when
making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1bc43fe-4596-4b43-bd44-412ca53fcd3c
📒 Files selected for processing (16)
backend/src/common/application/global-database-context.interface.tsbackend/src/common/application/global-database-context.tsbackend/src/entities/table-schema/application/data-structures/generate-schema-change.ds.tsbackend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.tsbackend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.extension.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/repository/schema-change-chat-message-repository.interface.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/schema-change-chat-message.entity.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat/repository/schema-change-chat-repository.extension.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat/repository/schema-change-chat-repository.interface.tsbackend/src/entities/table-schema/schema-change-chat/schema-change-chat/schema-change-chat.entity.tsbackend/src/entities/table-schema/table-schema.controller.tsbackend/src/entities/table-schema/table-schema.module.tsbackend/src/entities/table-schema/use-cases/generate-schema-change.use-case.tsbackend/src/migrations/1778767036234-AddSchemaChangeChatEntities.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-schema-postgres-e2e.test.ts
Comment on lines
+19
to
29
| @ApiProperty({ | ||
| type: String, | ||
| required: false, | ||
| nullable: true, | ||
| description: | ||
| 'Optional thread ID to continue an existing conversation. When supplied, prior turns are prepended to the AI prompt, giving the model context for iterative refinement (e.g. "now also add an index", "rename it to created_at"). Omit to start a fresh thread; the returned threadId can be passed back on the next call.', | ||
| }) | ||
| @IsOptional() | ||
| @IsUUID() | ||
| threadId?: string; | ||
| } |
There was a problem hiding this comment.
Align nullable API contract with TypeScript type.
@ApiProperty marks threadId as nullable, but the property type excludes null. This creates a type-level mismatch for consumers and internal mappings.
Proposed fix
- threadId?: string;
+ threadId?: string | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @ApiProperty({ | |
| type: String, | |
| required: false, | |
| nullable: true, | |
| description: | |
| 'Optional thread ID to continue an existing conversation. When supplied, prior turns are prepended to the AI prompt, giving the model context for iterative refinement (e.g. "now also add an index", "rename it to created_at"). Omit to start a fresh thread; the returned threadId can be passed back on the next call.', | |
| }) | |
| @IsOptional() | |
| @IsUUID() | |
| threadId?: string; | |
| } | |
| `@ApiProperty`({ | |
| type: String, | |
| required: false, | |
| nullable: true, | |
| description: | |
| 'Optional thread ID to continue an existing conversation. When supplied, prior turns are prepended to the AI prompt, giving the model context for iterative refinement (e.g. "now also add an index", "rename it to created_at"). Omit to start a fresh thread; the returned threadId can be passed back on the next call.', | |
| }) | |
| `@IsOptional`() | |
| `@IsUUID`() | |
| threadId?: string | null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/application/data-transfer-objects/generate-schema-change.dto.ts`
around lines 19 - 29, The DTO's `@ApiProperty` marks threadId as nullable but the
TypeScript declaration excludes null; update the property in
generate-schema-change.dto.ts so the TypeScript type allows null (e.g.,
threadId?: string | null) and keep the existing decorators (`@ApiProperty` with
nullable: true, `@IsOptional`(), `@IsUUID`()) so validation still skips
null/undefined and UUID validation runs for non-null values.
Comment on lines
+21
to
+22
| 'Conversation thread ID. Present when the request used or created a chat thread. Pass it back as the threadId query param on subsequent generate calls to continue the conversation with full prior context.', | ||
| }) |
There was a problem hiding this comment.
Fix response docs: threadId is not a query param in this contract.
The description currently instructs clients to send threadId as a query param, but the request DTO models it as a body field. This will confuse API consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/application/data-transfer-objects/schema-change-batch-response.dto.ts`
around lines 21 - 22, The response DTO's description for the threadId field
incorrectly tells clients to pass it as a query param; update the docstring for
the threadId property in SchemaChangeBatchResponseDto (the DTO where threadId is
declared) to state that threadId should be returned by the server and included
in subsequent requests in the request body (not as a query param), e.g.,
"Present when the request used or created a chat thread. Include this threadId
in the request body of subsequent generate calls to continue the conversation
with full prior context."
Comment on lines
+22
to
+23
| @Column({ nullable: true, default: null, type: 'enum', enum: MessageRole }) | ||
| role: MessageRole; |
There was a problem hiding this comment.
Enforce non-null role at persistence level.
saveMessage(...) requires role, but this entity allows null in DB. That permits invalid rows and can break role-based prompt reconstruction later.
Proposed fix
- `@Column`({ nullable: true, default: null, type: 'enum', enum: MessageRole })
+ `@Column`({ type: 'enum', enum: MessageRole })
role: MessageRole;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Column({ nullable: true, default: null, type: 'enum', enum: MessageRole }) | |
| role: MessageRole; | |
| `@Column`({ type: 'enum', enum: MessageRole }) | |
| role: MessageRole; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/schema-change-chat/schema-change-chat-message/schema-change-chat-message.entity.ts`
around lines 22 - 23, The entity column for role currently allows null even
though saveMessage(...) requires a role; change the Column on
schema-change-chat-message.entity (the role property of type MessageRole) to be
non-nullable (nullable: false) and remove the default null so the DB enforces
presence; additionally add a migration to backfill or reject existing null rows
(e.g., set a sensible default role or fail migration) and apply the schema
change so the database constraint matches the saveMessage(...) contract.
Comment on lines
+21
to
+22
| @Column({ default: null }) | ||
| name: string; |
There was a problem hiding this comment.
name nullability is inconsistent with write path.
createChatForUser stores name: null when omitted, but this entity declares name as non-null string without nullable: true. This mismatch can cause runtime insert errors and weakens type safety.
Proposed fix
- `@Column`({ default: null })
- name: string;
+ `@Column`({ nullable: true, default: null })
+ name: string | null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Column({ default: null }) | |
| name: string; | |
| `@Column`({ nullable: true, default: null }) | |
| name: string | null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/schema-change-chat/schema-change-chat/schema-change-chat.entity.ts`
around lines 21 - 22, The entity property "name" is declared as type string but
the Column decorator lacks nullable: true while createChatForUser writes name:
null; update the SchemaChangeChat entity by adding nullable: true to the `@Column`
on the name property (and/or adjust the TypeScript type to string | null) so the
DB mapping and the write path (createChatForUser) are consistent; ensure you
modify the Column decorator on the name field and the property type accordingly.
Comment on lines
+102
to
+106
| if (isNewChat) { | ||
| this.generateAndUpdateChatName(chat.id, userPrompt).catch((error) => { | ||
| Sentry.captureException(error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fire-and-forget async operation with request-scoped context may cause connection issues.
The generateAndUpdateChatName call at line 103 is fire-and-forget (not awaited), but GlobalDatabaseContext is request-scoped (Scope.REQUEST). If the HTTP request completes before the async operation finishes, the database context may be destroyed, causing the operation to fail with connection errors.
Potential solutions
Option 1 (simplest): Await the operation to ensure it completes before the request ends:
- if (isNewChat) {
- this.generateAndUpdateChatName(chat.id, userPrompt).catch((error) => {
- Sentry.captureException(error);
- });
- }
+ if (isNewChat) {
+ await this.generateAndUpdateChatName(chat.id, userPrompt);
+ }Note: This adds ~500ms-2s latency to the response. If acceptable, this is the safest fix.
Option 2 (better isolation): Queue the operation for a background worker or use a singleton-scoped service to handle the database write independently of the request lifecycle.
Option 3 (acceptable risk): Keep current behavior but add monitoring to track failures via Sentry. Document that chat name generation is best-effort and may occasionally fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts`
around lines 102 - 106, The call to generateAndUpdateChatName in the isNewChat
branch is fire-and-forget and can fail because GlobalDatabaseContext is
request-scoped and may be destroyed before the async work completes; either
await the call (await this.generateAndUpdateChatName(chat.id, userPrompt)) so
the request lifecycle keeps the DB context alive and handle errors with
try/catch + Sentry.captureException, or instead delegate the task to a
singleton/background worker (e.g., a ChatNameGenerationService.enqueue(chat.id,
userPrompt) or BackgroundJobService) that performs the DB write outside the
request scope; update the code that currently calls
this.generateAndUpdateChatName(...).catch(...) accordingly.
Comment on lines
+173
to
+189
| private async resolveChat( | ||
| threadId: string | null, | ||
| userId: string, | ||
| connectionId: string, | ||
| ): Promise<{ chat: SchemaChangeChatEntity; isNewChat: boolean }> { | ||
| if (threadId) { | ||
| const existing = await this._dbContext.schemaChangeChatRepository.findChatByIdAndUserId(threadId, userId); | ||
| if (existing) { | ||
| if (existing.connection_id !== connectionId) { | ||
| throw new BadRequestException('Provided threadId belongs to a different connection.'); | ||
| } | ||
| return { chat: existing, isNewChat: false }; | ||
| } | ||
| } | ||
| const created = await this._dbContext.schemaChangeChatRepository.createChatForUser(userId, connectionId); | ||
| return { chat: created, isNewChat: true }; | ||
| } |
There was a problem hiding this comment.
Invalid threadId silently creates a new chat instead of throwing an error.
When a user provides a threadId that doesn't exist (or belongs to a different user), the code falls through to line 187 and creates a new chat instead of throwing an error. This violates the principle of least surprise—the user expects to continue an existing conversation but unknowingly starts a new one.
🔧 Proposed fix
private async resolveChat(
threadId: string | null,
userId: string,
connectionId: string,
): Promise<{ chat: SchemaChangeChatEntity; isNewChat: boolean }> {
if (threadId) {
const existing = await this._dbContext.schemaChangeChatRepository.findChatByIdAndUserId(threadId, userId);
- if (existing) {
- if (existing.connection_id !== connectionId) {
- throw new BadRequestException('Provided threadId belongs to a different connection.');
- }
- return { chat: existing, isNewChat: false };
+ if (!existing) {
+ throw new NotFoundException('Chat thread not found.');
+ }
+ if (existing.connection_id !== connectionId) {
+ throw new BadRequestException('Provided threadId belongs to a different connection.');
}
+ return { chat: existing, isNewChat: false };
}
const created = await this._dbContext.schemaChangeChatRepository.createChatForUser(userId, connectionId);
return { chat: created, isNewChat: true };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private async resolveChat( | |
| threadId: string | null, | |
| userId: string, | |
| connectionId: string, | |
| ): Promise<{ chat: SchemaChangeChatEntity; isNewChat: boolean }> { | |
| if (threadId) { | |
| const existing = await this._dbContext.schemaChangeChatRepository.findChatByIdAndUserId(threadId, userId); | |
| if (existing) { | |
| if (existing.connection_id !== connectionId) { | |
| throw new BadRequestException('Provided threadId belongs to a different connection.'); | |
| } | |
| return { chat: existing, isNewChat: false }; | |
| } | |
| } | |
| const created = await this._dbContext.schemaChangeChatRepository.createChatForUser(userId, connectionId); | |
| return { chat: created, isNewChat: true }; | |
| } | |
| private async resolveChat( | |
| threadId: string | null, | |
| userId: string, | |
| connectionId: string, | |
| ): Promise<{ chat: SchemaChangeChatEntity; isNewChat: boolean }> { | |
| if (threadId) { | |
| const existing = await this._dbContext.schemaChangeChatRepository.findChatByIdAndUserId(threadId, userId); | |
| if (!existing) { | |
| throw new NotFoundException('Chat thread not found.'); | |
| } | |
| if (existing.connection_id !== connectionId) { | |
| throw new BadRequestException('Provided threadId belongs to a different connection.'); | |
| } | |
| return { chat: existing, isNewChat: false }; | |
| } | |
| const created = await this._dbContext.schemaChangeChatRepository.createChatForUser(userId, connectionId); | |
| return { chat: created, isNewChat: true }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts`
around lines 173 - 189, The resolveChat method currently creates a new chat when
a threadId is supplied but not found/owned by the user; change it so that if a
non-null threadId is provided and findChatByIdAndUserId returns no chat, you
throw a BadRequestException instead of falling through to createChatForUser.
Keep the existing connection_id check (existing.connection_id !== connectionId)
and only call createChatForUser when threadId is null; update the logic in
resolveChat to detect the "threadId supplied but missing" case and raise an
error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
threadIdin responses for seamless continuation of schema change conversations