fix: comprehensive schema normalization and constraint fixes#714
Conversation
- Fix updateDrawingContext: change message role from 'data' to 'tool' (messages_role_check only allowed user/assistant/system/tool) - Fix calendar.ts: change calendar note message role from 'data' to 'tool' (same constraint violation in saveNote path) - Create migration 0007: create missing prompt_generation_jobs table (schema defined it but table was never created in production) - Create migration 0008: add 'data' to messages_role_check as a valid role (preventive: future-proof the constraint if 'data' role is needed) All migrations use NOT VALID + VALIDATE pattern to avoid write blocking.
|
Someone is attempting to deploy a commit to the QCX-MAIN Team on Vercel. A member of the Team first needs to authorize it. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
WalkthroughAdds prompt-generation job persistence, permits the ChangesPrompt generation job storage
Message role persistence
Chat message identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
|
ngoiyaeric seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- msg.id can be string | undefined in the NewMessage type - Use nullish coalescing (??) to generate a UUID fallback - Fixes TypeScript build error: Type 'string | undefined' not assignable to 'string'
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/actions/chat-db.ts (1)
126-128: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winDedup suffix produces invalid UUIDs that will violate the
uuidcolumn constraint.When a duplicate
idis detected, the code appends-${Math.random().toString(36).substring(2, 8)}to the original UUID. This produces a string like550e8400-e29b-41d4-a716-446655440000-abc123, which is not a valid UUID. PostgreSQL'suuidcolumn type will reject this on insert, causing the entire transaction to fail.This is pre-existing, but it's directly in the path of the line 123 change — any
msg.idthat is defined but duplicated (the "groupeId" scenario described in the comment at line 117) will hit this branch.🔒️ Proposed fix: use `crypto.randomUUID()` for dedup collisions
while (seenIds.has(id)) { - id = `${id}-${Math.random().toString(36).substring(2, 8)}`; + id = crypto.randomUUID(); }This ensures all generated IDs remain valid UUIDs. If you need to preserve a deterministic relationship to the original ID for debugging, consider a UUID v5 (namespace-based) approach instead, but a fresh random UUID is the simplest correct fix.
🤖 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 `@lib/actions/chat-db.ts` around lines 126 - 128, The deduplication logic in chat-db.ts is producing invalid UUIDs by appending a random suffix to an existing id inside the seenIds collision loop. Update the duplicate-id handling in the chat insert path so the generated replacement remains a valid UUID, using crypto.randomUUID() (or an equivalent UUID-safe approach) in the branch that currently mutates id. Keep the fix localized to the id collision handling around the msg.id/groupId flow so inserts into the uuid column do not fail.lib/actions/calendar.ts (1)
92-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid double-encoding calendar tool payloads.
app/actions.tsxre-stringifies everytoolmessage before sending it toresearcher, so persistingJSON.stringify(...)here turns the calendar note into escaped JSON on the next pass. Serialize this payload only once, or skip the second stringify fortoolmessages that are already text.🤖 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 `@lib/actions/calendar.ts` around lines 92 - 102, The calendar tool payload is being serialized twice: `createMessage` in `calendar.ts` stores `content` as an already stringified JSON value, and `app/actions.tsx` stringifies `tool` messages again before sending them to `researcher`. Update the `calendarContextMessage` construction so the `tool` payload is stored in the form expected by the downstream flow, or adjust the `tool` handling in `app/actions.tsx` to avoid re-stringifying content that is already plain text. Use the `calendarContextMessage` and `createMessage` path to locate the fix.
🤖 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 `@lib/actions/chat.ts`:
- Around line 235-240: The drawing-context tool payload is being stringified
twice between chat message creation and the researcher path. Update the
`newDrawingMessage` flow in `chat.ts` and the `tool` handling in
`app/actions.tsx` so the payload is serialized in only one place and passed
through unchanged everywhere else. Use the existing `DbNewMessage`,
`newDrawingMessage`, and `researcher` tool-message path to keep the stored
content as the original JSON string instead of an escaped string.
---
Outside diff comments:
In `@lib/actions/calendar.ts`:
- Around line 92-102: The calendar tool payload is being serialized twice:
`createMessage` in `calendar.ts` stores `content` as an already stringified JSON
value, and `app/actions.tsx` stringifies `tool` messages again before sending
them to `researcher`. Update the `calendarContextMessage` construction so the
`tool` payload is stored in the form expected by the downstream flow, or adjust
the `tool` handling in `app/actions.tsx` to avoid re-stringifying content that
is already plain text. Use the `calendarContextMessage` and `createMessage` path
to locate the fix.
In `@lib/actions/chat-db.ts`:
- Around line 126-128: The deduplication logic in chat-db.ts is producing
invalid UUIDs by appending a random suffix to an existing id inside the seenIds
collision loop. Update the duplicate-id handling in the chat insert path so the
generated replacement remains a valid UUID, using crypto.randomUUID() (or an
equivalent UUID-safe approach) in the branch that currently mutates id. Keep the
fix localized to the id collision handling around the msg.id/groupId flow so
inserts into the uuid column do not fail.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 578f63ef-858b-4cd9-afb9-18f7610f8dac
📒 Files selected for processing (5)
drizzle/migrations/0007_create_prompt_generation_jobs.sqldrizzle/migrations/0008_allow_data_message_role.sqllib/actions/calendar.tslib/actions/chat-db.tslib/actions/chat.ts
📜 Review details
⚠️ CI failures not shown inline (1)
Commit Status: Vercel – qcx: Vercel – qcx
Conclusion: failure
Authorization required to deploy.
🧰 Additional context used
🪛 Squawk (2.59.0)
drizzle/migrations/0008_allow_data_message_role.sql
[warning] 12-12: Using NOT VALID and VALIDATE CONSTRAINT in the same transaction will block all reads while the constraint is validated. Add constraint as NOT VALID in one transaction and VALIDATE CONSTRAINT in a separate transaction.
(constraint-missing-not-valid)
drizzle/migrations/0007_create_prompt_generation_jobs.sql
[warning] 17-17: Using NOT VALID and VALIDATE CONSTRAINT in the same transaction will block all reads while the constraint is validated. Add constraint as NOT VALID in one transaction and VALIDATE CONSTRAINT in a separate transaction.
(constraint-missing-not-valid)
🔇 Additional comments (5)
drizzle/migrations/0007_create_prompt_generation_jobs.sql (2)
4-13: LGTM!
15-17: 🗄️ Data Integrity & Integration | 💤 Low valueSquawk
NOT VALID+VALIDATEwarning is a false positive here.The blocking concern only matters when validating a constraint against existing rows. Since
prompt_generation_jobsis created empty in this same migration (Line 4),VALIDATE CONSTRAINTscans zero rows and completes instantly, so the two-step split provides no benefit and causes no read-blocking. No change required; you could optionally inline the FK in theCREATE TABLEto reduce noise, but leaving it consistent with your other migrations is fine.Column and FK contract matches
lib/db/schema.ts(promptGenerationJobs) and theusers.idUUID target.Source: Linters/SAST tools
lib/actions/chat-db.ts (2)
123-123: UUID fallback for missing message IDs is correct.Generating a UUID before dedup tracking ensures all messages are uniformly handled by the
seenIdsset and theonConflictDoUpdatetarget. This properly addresses theundefinedmsg.idflow from the upstream caller inlib/actions/chat.ts.
123-123: 🔒 Security & Privacy
crypto.randomUUID()is fine here Bun exposes the Web Crypto API globally, so this bare reference works in the configured runtime.> Likely an incorrect or invalid review comment.drizzle/migrations/0008_allow_data_message_role.sql (1)
8-12: 🩺 Stability & AvailabilityVerify the validation step is isolated from the add-constraint step.
The file uses
NOT VALIDandVALIDATE CONSTRAINTback-to-back, and the static-analysis hint says this can prolong locking if the migration runner wraps the file in a single transaction. Please confirm the execution model; otherwise split validation into a follow-up migration.Source: Linters/SAST tools
| const newDrawingMessage: DbNewMessage = { | ||
| userId: userId, | ||
| chatId: chatId, | ||
| role: 'data', | ||
| role: 'tool', | ||
| content: JSON.stringify(contextData), | ||
| createdAt: new Date(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lib/actions/chat.ts (around lines 220-260) ---'
sed -n '220,260p' lib/actions/chat.ts
echo
echo '--- app/actions.tsx (around lines 360-410) ---'
sed -n '360,410p' app/actions.tsx
echo
echo '--- search for tool-message handling and JSON.stringify usage ---'
rg -n "role:\s*'tool'|role === 'tool'|JSON\.stringify\(msg\.content\)|content:\s*JSON\.stringify|CoreMessage|tool" lib app -g '!**/dist/**' -g '!**/build/**'Repository: QueueLab/QCX
Length of output: 14446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lib/actions/chat.ts (around lines 200-220) ---'
sed -n '200,220p' lib/actions/chat.ts
echo
echo '--- app/actions.tsx (around lines 150-180) ---'
sed -n '150,180p' app/actions.tsx
echo
echo '--- app/actions.tsx (around lines 660-710) ---'
sed -n '660,710p' app/actions.tsx
echo
echo '--- lib/types/index.ts (around lines 55-80) ---'
sed -n '55,80p' lib/types/index.tsRepository: QueueLab/QCX
Length of output: 4245
Avoid double-stringifying the drawing-context tool payload. app/actions.tsx:377-388 stringifies every tool message again before sending it to researcher, so the JSON string saved in lib/actions/chat.ts becomes an escaped string instead of the original payload. Keep the existing string as-is for this path, or stringify in one place only.
🤖 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 `@lib/actions/chat.ts` around lines 235 - 240, The drawing-context tool payload
is being stringified twice between chat message creation and the researcher
path. Update the `newDrawingMessage` flow in `chat.ts` and the `tool` handling
in `app/actions.tsx` so the payload is serialized in only one place and passed
through unchanged everywhere else. Use the existing `DbNewMessage`,
`newDrawingMessage`, and `researcher` tool-message path to keep the stored
content as the original JSON string instead of an escaped string.
Summary
This PR addresses all identified future failure modes from a comprehensive schema audit:
Code Fixes
updateDrawingContext role fix (
lib/actions/chat.ts)'data'to'tool'messages_role_checkconstraint only allowed: user, assistant, system, tool'data'would have crashed every drawing context saveCalendar note message role fix (
lib/actions/calendar.ts)'data''tool'for the same reasonDatabase Migrations
Migration 0007 — Create missing
prompt_generation_jobstableMigration 0008 — Add
'data'to messages_role_check'data', it will be availableAll Previous Fixes (Already Merged)
Applied to Production
All database changes have been applied directly to the QCX-BACKEND Supabase project via Supabase MCP.
Summary by CodeRabbit
New Features
Bug Fixes