Repository navigation
Drizzle db setup - #1
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new database layer has been established with PostgreSQL connection pooling via Changes
Poem
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/drizzle.config.ts`:
- Around line 5-10: The drizzle.config.ts currently passes ENV.DATABASE_URL!
directly into dbCredentials.url which can be undefined when Drizzle Kit runs;
add a runtime guard before returning defineConfig that checks ENV.DATABASE_URL
(the same way backend/src/db/index.ts does) and throw a clear Error like
"Missing DATABASE_URL env var" if it's falsy, then use the validated value in
dbCredentials.url; locate the defineConfig call and replace the direct
ENV.DATABASE_URL usage with the validated variable.
In `@backend/src/config/env.ts`:
- Line 7: The exported ENV value for DATABASE_URL is using the misspelled
process.env.DATABSE_URL which yields undefined; update the initializer to read
process.env.DATABASE_URL (fix the typo) so ENV.DATABASE_URL is populated
correctly for consumers such as the DB initialization code and drizzle
configuration.
In `@backend/src/db/index.ts`:
- Around line 7-9: The error message thrown when ENV.DATABASE_URL is missing is
misleading; update the thrown Error in the guard that checks ENV.DATABASE_URL to
reference "DATABASE_URL" (e.g., throw new Error("DATABASE_URL is not set in
environment variables")) so the message matches the actual environment variable
being checked (look for the ENV.DATABASE_URL check in this module).
- Line 26: The exported db is currently a plain object ({ client: pool, schema
}) but needs to be a Drizzle instance so downstream code can use Drizzle query
methods; replace the plain object export with a Drizzle initialization by
calling drizzle({ client: pool, schema }) and export that result as db, and
ensure the drizzle function is imported (e.g., import { drizzle } from the
appropriate Drizzle node-postgres package) so symbols db, pool, schema, and
drizzle are used consistently.
In `@backend/src/db/schema.ts`:
- Around line 59-62: The product relation in commentsRelations is using the
wrong source field; change the product relation to reference comments.productId
instead of comments.userId so fields: [comments.productId] and references:
[products.id] are used (keep the user relation as-is referencing comments.userId
-> users.id); update the relation definition in commentsRelations to use the
comments.productId foreign key for the products relation.
- Around line 21-22: The products table's updatedAt column is missing Drizzle's
runtime update hook so updates don't refresh the timestamp; mirror
users.updatedAt by adding the $onUpdate hook to the products.updatedAt column
(e.g., products.updatedAt.$onUpdate(() => new Date())) so Drizzle will set the
updated_at on modifications; update the timestamp definition for updatedAt in
the products schema to include $onUpdate() matching the users.updatedAt pattern.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fdb87f1-d24e-44a5-9a86-8180f7be32df
📒 Files selected for processing (4)
backend/drizzle.config.tsbackend/src/config/env.tsbackend/src/db/index.tsbackend/src/db/schema.ts
| export default defineConfig({ | ||
| schema: "./src/db/schema.ts", | ||
| dialect: "postgresql", | ||
| dbCredentials: { | ||
| url: ENV.DATABASE_URL!, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/drizzle.config.tsRepository: nirajmaharjan783/productify
Length of output: 384
🏁 Script executed:
find . -type f -name "env.ts" -o -name "env.js" | head -20Repository: nirajmaharjan783/productify
Length of output: 98
🏁 Script executed:
cat -n src/config/env.tsRepository: nirajmaharjan783/productify
Length of output: 120
🏁 Script executed:
cat -n backend/src/config/env.tsRepository: nirajmaharjan783/productify
Length of output: 507
🏁 Script executed:
head -50 backend/src/config/env.tsRepository: nirajmaharjan783/productify
Length of output: 423
🏁 Script executed:
rg "DATABSE_URL|DATABASE_URL" --no-headingRepository: nirajmaharjan783/productify
Length of output: 332
🏁 Script executed:
rg "ENV\.DATABASE_URL" --no-heading -A 2 -B 2Repository: nirajmaharjan783/productify
Length of output: 736
Add runtime guard for DATABASE_URL in the CLI config to fail fast with Drizzle Kit commands.
Drizzle Kit commands (pull, push, migrate) read dbCredentials.url from drizzle.config.ts independently of runtime initialization. Currently, the file passes ENV.DATABASE_URL without validation, allowing undefined values to reach Drizzle. Add an explicit check here to match the guard already in place in backend/src/db/index.ts and ensure clear error messages when the environment variable is missing.
🛡️ Proposed fix
import { defineConfig } from "drizzle-kit"
import { ENV } from "./src/config/env"
+if (!ENV.DATABASE_URL) {
+ throw new Error("DATABASE_URL is not set in environment variables")
+}
export default defineConfig({
schema: "./src/db/schema.ts",
dialect: "postgresql",
dbCredentials: {
- url: ENV.DATABASE_URL!,
+ url: ENV.DATABASE_URL,
}
})📝 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.
| export default defineConfig({ | |
| schema: "./src/db/schema.ts", | |
| dialect: "postgresql", | |
| dbCredentials: { | |
| url: ENV.DATABASE_URL!, | |
| } | |
| import { defineConfig } from "drizzle-kit" | |
| import { ENV } from "./src/config/env" | |
| if (!ENV.DATABASE_URL) { | |
| throw new Error("DATABASE_URL is not set in environment variables") | |
| } | |
| export default defineConfig({ | |
| schema: "./src/db/schema.ts", | |
| dialect: "postgresql", | |
| dbCredentials: { | |
| url: ENV.DATABASE_URL, | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/drizzle.config.ts` around lines 5 - 10, The drizzle.config.ts
currently passes ENV.DATABASE_URL! directly into dbCredentials.url which can be
undefined when Drizzle Kit runs; add a runtime guard before returning
defineConfig that checks ENV.DATABASE_URL (the same way backend/src/db/index.ts
does) and throw a clear Error like "Missing DATABASE_URL env var" if it's falsy,
then use the validated value in dbCredentials.url; locate the defineConfig call
and replace the direct ENV.DATABASE_URL usage with the validated variable.
| if (!ENV.DATABASE_URL) { | ||
| throw new Error("DB_URL is not set in environment variables") | ||
| } |
There was a problem hiding this comment.
Rename the missing-env error to DATABASE_URL.
The guard is checking ENV.DATABASE_URL, but the thrown message still says DB_URL. That will point anyone debugging this failure at the wrong variable name.
✏️ Proposed fix
if (!ENV.DATABASE_URL) {
- throw new Error("DB_URL is not set in environment variables")
+ throw new Error("DATABASE_URL is not set in environment variables")
}📝 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.
| if (!ENV.DATABASE_URL) { | |
| throw new Error("DB_URL is not set in environment variables") | |
| } | |
| if (!ENV.DATABASE_URL) { | |
| throw new Error("DATABASE_URL is not set in environment variables") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/db/index.ts` around lines 7 - 9, The error message thrown when
ENV.DATABASE_URL is missing is misleading; update the thrown Error in the guard
that checks ENV.DATABASE_URL to reference "DATABASE_URL" (e.g., throw new
Error("DATABASE_URL is not set in environment variables")) so the message
matches the actual environment variable being checked (look for the
ENV.DATABASE_URL check in this module).
| createdAt: timestamp("created_at", { mode: "date" }).notNull().defaultNow(), | ||
| updatedAt: timestamp("updated_at", { mode: "date" }).notNull().defaultNow() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "schema.ts" -path "*/backend/src/db/*" | head -5Repository: nirajmaharjan783/productify
Length of output: 97
🏁 Script executed:
cat -n backend/src/db/schema.ts | head -30Repository: nirajmaharjan783/productify
Length of output: 1716
🌐 Web query:
Drizzle ORM $onUpdate timestamp behavior defaultNow
💡 Result:
In Drizzle ORM, the $onUpdate (or $onUpdateFn) method on a column defines a function that executes at runtime during UPDATE queries. The returned value automatically sets the column if no explicit value is provided in the .set object. Key behavior with timestamps and defaultNow: - If the column has .defaultNow (or .default/.defaultFn), $onUpdate applies only on updates. - If no default is set, $onUpdate also applies on inserts. - This is runtime-only; it does not affect schema generation or migrations via drizzle-kit. - Common pattern for updatedAt: timestamp('updated_at').defaultNow.$onUpdate( => new Date) or . $onUpdate( => sqlCURRENT_TIMESTAMP) for DB time. - Works across PostgreSQL, MySQL, SQLite. - Early bugs with sql... in $onUpdate fixed in later versions (e.g., PR #2911). Example: updatedAt: timestamp('updated_at', { mode: 'date' }).defaultNow.$onUpdate( => new Date), On update: db.update(table).set({ name: 'new' }) // updatedAt auto-updates, no need to specify it.
Citations:
- 1: Feat: add an
$onUpdatemethod to columns drizzle-team/drizzle-orm#1509 - 2: https://orm.drizzle.team/docs/latest-releases/drizzle-orm-v0305
- 3: https://stackoverflow.com/questions/77492088/is-timestamp-in-postgresqldrizzle-orm-available-to-store-and-update-to-new-dat
- 4: Fix $onUpdate not handling sql values drizzle-team/drizzle-orm#2911
Add $onUpdate() to products.updatedAt to sync updates.
users.updatedAt opts into Drizzle's runtime update hook via $onUpdate(), but products.updatedAt does not. Without it, product rows updated through Drizzle will retain their insert-time updated_at instead of reflecting the last modification.
- updatedAt: timestamp("updated_at", { mode: "date" }).notNull().defaultNow()
+ updatedAt: timestamp("updated_at", { mode: "date" }).notNull().defaultNow().$onUpdate(() => new Date())📝 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.
| createdAt: timestamp("created_at", { mode: "date" }).notNull().defaultNow(), | |
| updatedAt: timestamp("updated_at", { mode: "date" }).notNull().defaultNow() | |
| createdAt: timestamp("created_at", { mode: "date" }).notNull().defaultNow(), | |
| updatedAt: timestamp("updated_at", { mode: "date" }).notNull().defaultNow().$onUpdate(() => new Date()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/db/schema.ts` around lines 21 - 22, The products table's
updatedAt column is missing Drizzle's runtime update hook so updates don't
refresh the timestamp; mirror users.updatedAt by adding the $onUpdate hook to
the products.updatedAt column (e.g., products.updatedAt.$onUpdate(() => new
Date())) so Drizzle will set the updated_at on modifications; update the
timestamp definition for updatedAt in the products schema to include $onUpdate()
matching the users.updatedAt pattern.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/src/db/queries.ts (1)
31-36: Consider using Drizzle'sonConflictDoUpdatefor atomic upsert.The current implementation has a race condition: between
getUserByIdandcreateUser, another request could insert the same user, causing a unique constraint violation. Drizzle ORM supports atomic upserts viaonConflictDoUpdate.♻️ Suggested atomic upsert implementation
-// upsert => create or update -export const upsertUser = async (data: NewUser) => { - const existingUser = await getUserById(data.id) - if (existingUser) return updateUser(data.id, data) - - return createUser(data) -} +// upsert => create or update (atomic) +export const upsertUser = async (data: NewUser) => { + const [user] = await db + .insert(users) + .values(data) + .onConflictDoUpdate({ + target: users.id, + set: data, + }) + .returning() + return user +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/db/queries.ts` around lines 31 - 36, The current upsertUser implementation does a separate getUserById then createUser/updateUser which has a race condition; replace it with a single atomic upsert using Drizzle's onConflictDoUpdate on the unique id (use the same table/model used by createUser/updateUser) so the insert will update existing rows on conflict. Modify upsertUser to call the Drizzle insert with .onConflictDoUpdate({ target: userTable.id, set: { /* map fields from NewUser that should be updated */ } }) and remove the getUserById/createUser/updateUser branching so the operation is atomic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/db/queries.ts`:
- Around line 66-72: Rename the exported function getProductsByuserId to
getProductsByUserId to fix the camelCase inconsistency; update the function
declaration/export symbol from getProductsByuserId to getProductsByUserId and
adjust all references/imports/usages across the codebase (tests, call sites, and
any index re-exports) to use getProductsByUserId so nothing breaks at runtime.
- Around line 91-94: Rename the exported function deleteComments to
deleteComment (it deletes a single comment by id) and update its export
signature accordingly; inside the function keep the implementation using
db.delete(comments).where(eq(comments.id, id)).returning() and returning the
single comment variable, then update all call sites/imports that reference
deleteComments to the new name deleteComment and adjust any tests or type
annotations that expect the plural name.
- Around line 96-101: The function getCommentById currently awaits
db.query.comments.findFirst but does not return its result, so callers get
undefined; fix it by adding a return before the await in getCommentById so the
resolved comment (including the with: { user: true } relation) is returned to
callers, ensuring any downstream code receives the query result.
---
Nitpick comments:
In `@backend/src/db/queries.ts`:
- Around line 31-36: The current upsertUser implementation does a separate
getUserById then createUser/updateUser which has a race condition; replace it
with a single atomic upsert using Drizzle's onConflictDoUpdate on the unique id
(use the same table/model used by createUser/updateUser) so the insert will
update existing rows on conflict. Modify upsertUser to call the Drizzle insert
with .onConflictDoUpdate({ target: userTable.id, set: { /* map fields from
NewUser that should be updated */ } }) and remove the
getUserById/createUser/updateUser branching so the operation is atomic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48eb53b0-dfb1-4bde-8c50-8fee6375d2de
📒 Files selected for processing (2)
backend/src/db/index.tsbackend/src/db/queries.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/db/index.ts
| export const getProductsByuserId = async (userId: string) => { | ||
| return db.query.products.findMany({ | ||
| where: eq(products.userId, userId), | ||
| with: { user: true }, | ||
| orderBy: (products, { desc }) => [desc(products.createdAt)] | ||
| }) | ||
| } |
There was a problem hiding this comment.
Inconsistent function naming: getProductsByuserId should be getProductsByUserId.
The lowercase 'u' in userId breaks camelCase consistency with other functions like getUserById and getProductById.
✏️ Proposed fix
-export const getProductsByuserId = async (userId: string) => {
+export const getProductsByUserId = async (userId: string) => {📝 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.
| export const getProductsByuserId = async (userId: string) => { | |
| return db.query.products.findMany({ | |
| where: eq(products.userId, userId), | |
| with: { user: true }, | |
| orderBy: (products, { desc }) => [desc(products.createdAt)] | |
| }) | |
| } | |
| export const getProductsByUserId = async (userId: string) => { | |
| return db.query.products.findMany({ | |
| where: eq(products.userId, userId), | |
| with: { user: true }, | |
| orderBy: (products, { desc }) => [desc(products.createdAt)] | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/db/queries.ts` around lines 66 - 72, Rename the exported function
getProductsByuserId to getProductsByUserId to fix the camelCase inconsistency;
update the function declaration/export symbol from getProductsByuserId to
getProductsByUserId and adjust all references/imports/usages across the codebase
(tests, call sites, and any index re-exports) to use getProductsByUserId so
nothing breaks at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/db/queries.ts`:
- Around line 95-98: The updateProduct function currently returns undefined when
no product matches; make it consistent with updateUser/deleteProduct by throwing
an explicit error when not found. After awaiting
db.update(products).set(data).where(eq(products.id, id)).returning(), check the
returned product (the variable product) and if falsy throw a descriptive error
(e.g., "Product not found" or the project's NotFound error type) otherwise
return the product; update callers/tests if they expect the error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47c03d37-03f0-4b2d-b16e-476bf00b8055
📒 Files selected for processing (3)
backend/src/config/env.tsbackend/src/db/queries.tsbackend/src/db/schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/config/env.ts
| export const updateProduct = async (id: string, data: Partial<NewProduct>) => { | ||
| const [product] = await db.update(products).set(data).where(eq(products.id, id)).returning() | ||
| return product | ||
| } |
There was a problem hiding this comment.
updateProduct silently returns undefined when product doesn't exist.
Unlike updateUser (lines 25-32) and deleteProduct (lines 100-107) which check existence and throw an error, updateProduct will return undefined if no matching product is found. This inconsistency could cause issues for callers expecting an error on missing products.
🔧 Proposed fix for consistency
export const updateProduct = async (id: string, data: Partial<NewProduct>) => {
+ const existingProduct = await getProductById(id)
+ if (!existingProduct) {
+ throw new Error(`Product with id ${id} not found`)
+ }
const [product] = await db.update(products).set(data).where(eq(products.id, id)).returning()
return product
}📝 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.
| export const updateProduct = async (id: string, data: Partial<NewProduct>) => { | |
| const [product] = await db.update(products).set(data).where(eq(products.id, id)).returning() | |
| return product | |
| } | |
| export const updateProduct = async (id: string, data: Partial<NewProduct>) => { | |
| const existingProduct = await getProductById(id) | |
| if (!existingProduct) { | |
| throw new Error(`Product with id ${id} not found`) | |
| } | |
| const [product] = await db.update(products).set(data).where(eq(products.id, id)).returning() | |
| return product | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/db/queries.ts` around lines 95 - 98, The updateProduct function
currently returns undefined when no product matches; make it consistent with
updateUser/deleteProduct by throwing an explicit error when not found. After
awaiting db.update(products).set(data).where(eq(products.id, id)).returning(),
check the returned product (the variable product) and if falsy throw a
descriptive error (e.g., "Product not found" or the project's NotFound error
type) otherwise return the product; update callers/tests if they expect the
error.
|
done |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes