Conversation
kibertoad
requested review from
a team,
CarlosGamero and
dariacm
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lokalise/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds MySQL support and introduces a new Sequence DiagramsequenceDiagram
actor Caller
participant FileSystem as File System
participant DB as Database
Caller->>Caller: markMigrationsApplied(options)
Caller->>FileSystem: Read meta/_journal.json
FileSystem-->>Caller: Journal metadata + entries
Caller->>FileSystem: Read migration SQL files<br/>(.sql for each entry)
FileSystem-->>Caller: SQL content
Caller->>Caller: Compute SHA-256 hash<br/>for each migration
Caller->>DB: Query existing<br/>tracked migration hashes
DB-->>Caller: Existing hashes
Caller->>Caller: Determine missing<br/>migrations
Caller->>DB: Insert missing migrations<br/>with timestamps
DB-->>Caller: Insert complete
Caller-->>Caller: Return result with<br/>applied/skipped counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (6)
packages/app/drizzle-utils/src/markMigrationsApplied.ts (3)
96-101: Identifier quoting does not escape special characters.The
quoteIdentifierfunction wraps identifiers in quotes but doesn't escape embedded quote characters. IfmigrationsTableormigrationsSchemacontain backticks (MySQL) or double quotes (PostgreSQL), it could break the SQL or enable injection.While this is a migration utility (not a web API) and input comes from trusted configuration, consider escaping for defense in depth.
🛡️ Suggested defensive fix
function quoteIdentifier(name: string, dialect: Dialect): string { if (dialect === 'mysql') { - return `\`${name}\`` + return `\`${name.replace(/`/g, '``')}\`` } - return `"${name}"` + return `"${name.replace(/"/g, '""')}"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/src/markMigrationsApplied.ts` around lines 96 - 101, The quoteIdentifier function currently wraps names but doesn't escape embedded quote characters, so update quoteIdentifier(name: string, dialect: Dialect) to first replace any internal quote characters with the SQL-appropriate escaped form (for mysql replace backtick ` with double backticks ``; for postgres/other dialects replace double-quote " with two double-quotes ""), then wrap the result in the chosen quote character; ensure callers that pass migrationsTable and migrationsSchema use this updated function so identifiers containing quotes are safely escaped.
176-179: Journal file is read twice.
readMigrationJournalis called at line 177, thenreadMigrationEntriesinternally calls it again at line 179. Consider refactoring to pass the already-parsed journal to avoid redundant file I/O.♻️ Suggested optimization
Add an internal helper that accepts a pre-parsed journal:
function readMigrationEntriesFromJournal( folder: string, journal: MigrationJournal, ): MigrationEntry[] { return journal.entries.map((entry) => { const sqlPath = join(folder, `${entry.tag}.sql`) const sqlContent = readFileSync(sqlPath, 'utf-8') return { tag: entry.tag, hash: computeMigrationHash(sqlContent), createdAt: entry.when, } }) }Then use it in
markMigrationsApplied:const journal = readMigrationJournal(folder) const dialect = resolveDialect(journal, options.dialect) - const entries = readMigrationEntries(folder) + const entries = readMigrationEntriesFromJournal(folder, journal)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/src/markMigrationsApplied.ts` around lines 176 - 179, The code currently calls readMigrationJournal(folder) in markMigrationsApplied and then calls readMigrationEntries(folder), which itself re-reads and parses the journal causing duplicate I/O; refactor by adding a helper like readMigrationEntriesFromJournal(folder, journal) (or modify readMigrationEntries to accept an optional MigrationJournal) and use the already-parsed journal returned by readMigrationJournal inside markMigrationsApplied to build entries (compute hash, read SQL files, map createdAt/tag) so the journal file is read only once; update callers to pass the pre-parsed journal and remove the redundant readMigrationJournal call inside readMigrationEntries.
115-119: Synchronous file I/O may block the event loop.
readFileSyncis used for reading the journal and SQL files. For typical migration scenarios with a small number of files, this is acceptable. However, if this utility might be used in contexts with many migrations or performance-sensitive environments, consider offering an async variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/src/markMigrationsApplied.ts` around lines 115 - 119, The current readMigrationJournal function (and other places using readFileSync to load SQL/journal files) performs synchronous file I/O which can block the event loop; add an asynchronous variant (e.g., readMigrationJournalAsync) that uses fs.promises.readFile (or util.promisify) and returns Promise<MigrationJournal>, and update any helpers that read SQL files to provide async counterparts (e.g., readSqlFileAsync) so callers can opt into non-blocking behavior while keeping the existing sync implementations for backward compatibility; ensure the async functions preserve the same path resolution logic (join(resolve(migrationsFolder), 'meta', '_journal.json')) and JSON.parse behavior and export them alongside the current sync functions.packages/app/drizzle-utils/README.md (1)
94-114: Add language specifier to fenced code block.The workflow steps are in a fenced code block without a language specifier. Consider using
textorplaintextas the language, or reformatting as a numbered markdown list for better accessibility and linting compliance.📝 Suggested fix
-``` +```text 1. npm install drizzle-orm drizzle-kit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/README.md` around lines 94 - 114, The fenced code block containing the numbered Drizzle workflow (starting with "1. npm install drizzle-orm drizzle-kit") lacks a language specifier; update that triple-backtick block to include a language such as text or plaintext (e.g., ```text) or alternatively convert the block into a proper Markdown numbered list so linters and accessibility tools recognize it; ensure the visible steps (npm install..., Create your Drizzle schema, npx drizzle-kit generate, Run markMigrationsApplied(...), npx drizzle-kit migrate, Remove the old ORM...) remain unchanged in content.packages/app/drizzle-utils/src/markMigrationsApplied.test.ts (2)
280-290: Consider usingafterEachfor MySQL connection cleanup.The current pattern closes
connectioninside individual tests (lines 311, 339), which works but is inconsistent. If a test fails before callingconnection.end(), the connection may leak. Consider moving cleanup toafterEachfor robustness.♻️ Suggested refactor
beforeEach(async () => { connection = await mysql.createConnection(getMysqlDatabaseUrl()) await connection.execute(`DROP TABLE IF EXISTS \`${testTable}\``) }) + afterEach(async () => { + if (connection) { + await connection.end() + } + }) + afterAll(async () => { // Clean up - connection may have been closed by a test, create fresh one const conn = await mysql.createConnection(getMysqlDatabaseUrl()) await conn.execute(`DROP TABLE IF EXISTS \`${testTable}\``) await conn.end() })Then remove
await connection.end()from individual tests (lines 311, 339).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/src/markMigrationsApplied.test.ts` around lines 280 - 290, Tests currently rely on closing the MySQL connection inside individual tests which can leak if a test fails; add an afterEach hook to always drop the test table and call connection.end() (mirror the setup in beforeEach that creates connection using mysql.createConnection and getMysqlDatabaseUrl()), and remove the explicit await connection.end() calls from the individual tests (references: beforeEach, afterAll, and the tests that currently call connection.end()). This ensures connection cleanup runs after every test and prevents leaks while preserving the existing beforeEach table-drop/setup logic.
165-183: Consider cleaning up the temporary directory.The test creates a temporary directory but doesn't clean it up afterward. While the OS will eventually clear
tmpdir(), explicitly removing it ensures a cleaner test environment.♻️ Suggested improvement
+ const { mkdtempSync, writeFileSync, mkdirSync, rmSync } = await import('node:fs') const { tmpdir } = await import('node:os') const tempDir = mkdtempSync(join(tmpdir(), 'drizzle-test-')) mkdirSync(join(tempDir, 'meta'), { recursive: true }) writeFileSync( join(tempDir, 'meta', '_journal.json'), JSON.stringify({ version: '7', dialect: 'postgresql', entries: [] }), ) const result = await markMigrationsApplied({ migrationsFolder: tempDir, executor: noopExecutor, dialect: 'postgresql', }) expect(result).toEqual({ total: 0, applied: 0, skipped: 0, entries: [] }) + + // Cleanup + rmSync(tempDir, { recursive: true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/drizzle-utils/src/markMigrationsApplied.test.ts` around lines 165 - 183, The test creates a temporary directory (tempDir via mkdtempSync) for markMigrationsApplied but never removes it; update the test to remove the tempDir after the assertion (e.g., in a finally block or afterEach) using fs.rmSync or fs.promises.rm with recursive: true and force: true so the created meta/_journal.json and tempDir are cleaned up; reference the tempDir variable and the markMigrationsApplied invocation (and noopExecutor) when adding the cleanup to ensure the temp files are deleted regardless of test success or failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/drizzle-utils/package.json`:
- Line 30: The pretest:ci npm script uses the Docker Compose flag --quiet-pull
which requires Docker Compose v2.0.0+ (or legacy v1.20.0+); update the
"pretest:ci" script to remove or make the flag conditional so it doesn't break
older CI runners: either drop --quiet-pull from the "pretest:ci" entry in
package.json, or replace the single command with a small shell-safe sequence
that checks docker compose version (or attempts a plain docker compose pull/up
fallback) before running docker compose up -d --wait; ensure references to
"pretest:ci" and the --quiet-pull token are updated accordingly.
In `@packages/app/drizzle-utils/src/markMigrationsApplied.test.ts`:
- Around line 43-45: The TS2532 arises because TypeScript can't guarantee
journal.entries is defined after expect(journal.entries).toHaveLength(2); fix by
asserting or guarding before indexing: either add an explicit non-null assertion
when accessing items (e.g. use journal.entries![0] and journal.entries![1]) or
add an assertion like expect(journal.entries).toBeDefined() /
expect(journal.entries).not.toBeUndefined() before the indexed expects so the
type narrows; update the assertions referencing journal.entries in
markMigrationsApplied.test.ts accordingly.
In `@packages/app/drizzle-utils/test/getMysqlDatabaseUrl.ts`:
- Around line 4-10: The current check uses
process.env[MYSQL_DATABASE_URL_ENVVAR] and throws only when it's undefined,
allowing whitespace-only values to slip through; update the logic in
getMysqlDatabaseUrl (use the MYSQL_DATABASE_URL_ENVVAR and databaseUrl
variables) to trim the environment value first (e.g., const trimmed =
(process.env[MYSQL_DATABASE_URL_ENVVAR] ?? "").trim()), then throw an Error if
trimmed is empty, and finally return the trimmed value so blank-but-present DB
URLs are rejected early with a clear message.
---
Nitpick comments:
In `@packages/app/drizzle-utils/README.md`:
- Around line 94-114: The fenced code block containing the numbered Drizzle
workflow (starting with "1. npm install drizzle-orm drizzle-kit") lacks a
language specifier; update that triple-backtick block to include a language such
as text or plaintext (e.g., ```text) or alternatively convert the block into a
proper Markdown numbered list so linters and accessibility tools recognize it;
ensure the visible steps (npm install..., Create your Drizzle schema, npx
drizzle-kit generate, Run markMigrationsApplied(...), npx drizzle-kit migrate,
Remove the old ORM...) remain unchanged in content.
In `@packages/app/drizzle-utils/src/markMigrationsApplied.test.ts`:
- Around line 280-290: Tests currently rely on closing the MySQL connection
inside individual tests which can leak if a test fails; add an afterEach hook to
always drop the test table and call connection.end() (mirror the setup in
beforeEach that creates connection using mysql.createConnection and
getMysqlDatabaseUrl()), and remove the explicit await connection.end() calls
from the individual tests (references: beforeEach, afterAll, and the tests that
currently call connection.end()). This ensures connection cleanup runs after
every test and prevents leaks while preserving the existing beforeEach
table-drop/setup logic.
- Around line 165-183: The test creates a temporary directory (tempDir via
mkdtempSync) for markMigrationsApplied but never removes it; update the test to
remove the tempDir after the assertion (e.g., in a finally block or afterEach)
using fs.rmSync or fs.promises.rm with recursive: true and force: true so the
created meta/_journal.json and tempDir are cleaned up; reference the tempDir
variable and the markMigrationsApplied invocation (and noopExecutor) when adding
the cleanup to ensure the temp files are deleted regardless of test success or
failure.
In `@packages/app/drizzle-utils/src/markMigrationsApplied.ts`:
- Around line 96-101: The quoteIdentifier function currently wraps names but
doesn't escape embedded quote characters, so update quoteIdentifier(name:
string, dialect: Dialect) to first replace any internal quote characters with
the SQL-appropriate escaped form (for mysql replace backtick ` with double
backticks ``; for postgres/other dialects replace double-quote " with two
double-quotes ""), then wrap the result in the chosen quote character; ensure
callers that pass migrationsTable and migrationsSchema use this updated function
so identifiers containing quotes are safely escaped.
- Around line 176-179: The code currently calls readMigrationJournal(folder) in
markMigrationsApplied and then calls readMigrationEntries(folder), which itself
re-reads and parses the journal causing duplicate I/O; refactor by adding a
helper like readMigrationEntriesFromJournal(folder, journal) (or modify
readMigrationEntries to accept an optional MigrationJournal) and use the
already-parsed journal returned by readMigrationJournal inside
markMigrationsApplied to build entries (compute hash, read SQL files, map
createdAt/tag) so the journal file is read only once; update callers to pass the
pre-parsed journal and remove the redundant readMigrationJournal call inside
readMigrationEntries.
- Around line 115-119: The current readMigrationJournal function (and other
places using readFileSync to load SQL/journal files) performs synchronous file
I/O which can block the event loop; add an asynchronous variant (e.g.,
readMigrationJournalAsync) that uses fs.promises.readFile (or util.promisify)
and returns Promise<MigrationJournal>, and update any helpers that read SQL
files to provide async counterparts (e.g., readSqlFileAsync) so callers can opt
into non-blocking behavior while keeping the existing sync implementations for
backward compatibility; ensure the async functions preserve the same path
resolution logic (join(resolve(migrationsFolder), 'meta', '_journal.json')) and
JSON.parse behavior and export them alongside the current sync functions.
🪄 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: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: acfa4b10-bdfd-4197-831f-6583e3146b38
📒 Files selected for processing (13)
packages/app/drizzle-utils/.env.testpackages/app/drizzle-utils/README.mdpackages/app/drizzle-utils/docker-compose.ymlpackages/app/drizzle-utils/package.jsonpackages/app/drizzle-utils/src/index.tspackages/app/drizzle-utils/src/markMigrationsApplied.test.tspackages/app/drizzle-utils/src/markMigrationsApplied.tspackages/app/drizzle-utils/test/fixtures/migrations-mysql/0000_init.sqlpackages/app/drizzle-utils/test/fixtures/migrations-mysql/meta/_journal.jsonpackages/app/drizzle-utils/test/fixtures/migrations/0000_init.sqlpackages/app/drizzle-utils/test/fixtures/migrations/0001_add_users.sqlpackages/app/drizzle-utils/test/fixtures/migrations/meta/_journal.jsonpackages/app/drizzle-utils/test/getMysqlDatabaseUrl.ts
CarlosGamero
approved these changes
Mar 28, 2026
Collaborator
CarlosGamero
left a comment
CarlosGamero
left a comment
There was a problem hiding this comment.
Looks great! just left some small comments 🚀 The only relevant one is about Prisma updatedAt prop
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
2 participants
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.
Changes
Provide a clear path for migration from Prisma to Drizzle
Checklist
major,minor,patchorskip-releaseAI Assistance Tracking
We're running a metric to understand where AI assists our engineering work. Please select exactly one of the options below:
Mark "Yes" if AI helped in any part of this work, for example: generating code, refactoring, debugging support,
explaining something, reviewing an idea, or suggesting an approach.